Jump to content
Rollo62

BestPractices: To raise, or not to raise ... an Exception in a class constructor

Recommended Posts

1 hour ago, Wagner Landgraf said:

Wow, what a long thread and discussion for something "simple". Use the 25-year old pattern for Delphi memory management:

Right, I'm also wondering how such a "simple" question touches so many basic language concepts.

 

But to clarify the original question again:

It was not about if raising exception in Create is possible and how to do that and catch it, the question was:   

"Shall I raise exceptions on purpose, or better not".

 

1 hour ago, Darian Miller said:

Also consider passing items into a constructor rather than creating items within it to increase testability.  

Yes, this is exactly what I am looking for, especially here I should raise exceptions if something is wrong with these parameters.

 

Thanks to all, who brought so many different aspects into this discussion.

 

I came already to the conclusion:

Raising on purpose in Create() is fine, especially to guard against missing or faulty injected parameters.

 

Everything else is just an intermediate state to the final, perfect design, but there is no real reason to keep such intermediates.

( only I'm not so sure if I am ready to "finalize" all my classes in time :classic_biggrin: )

 

  • Like 1

Share this post


Link to post
2 hours ago, Wagner Landgraf said:

Wow, what a long thread and discussion for something "simple". Use the 25-year old pattern for Delphi memory management:

 

Can't see a damn question about memory management. Maybe I'm reading a different thread.

Share this post


Link to post
21 minutes ago, Rollo62 said:

Raising on purpose in Create() is fine

It is.

 

21 minutes ago, Rollo62 said:

especially to guard against missing or faulty injected parameters.

like if Parameter1 = null then raise...?

What if that object is freed before you will use that in some of the methods?

What is the advantage of this early-validation? It's just flow control in this case.

Share this post


Link to post
20 minutes ago, Attila Kovacs said:

like if Parameter1 = null then raise...?

What if that object is freed before you will use that in some of the methods?

That external parameter is not in the domain of the class, so I have to take care outside, to keep everything clean.

 

Or do you mean issues with some kind of delayed access, in anonymous procs, events or threads ?

When the object might be released, while the event proc might still try to touch it.

I think this could has to be guarded by other means, its not related to the constructor exceptions.

 

20 minutes ago, Attila Kovacs said:

Whais the advantage of this early-validation? It's just flow control in this case.

I would not need Guards on every internal operation, to ensure all parameters are OK.

I would hardly see any strange sideeffects later on, when an invalid parameter causes issues only under rare conditions, the creation fails gracefully immediately before that happens.

 

Share this post


Link to post
1 hour ago, Attila Kovacs said:

Can't see a damn question about memory management. Maybe I'm reading a different thread.

I see lots of discussions about how to properly structure try..finally statements and how it behaves depending if a constructor raises an exception or not; if the object should be created inside or outside the try..finally, if the destructor should have a try..finally inside it, etc.

Maybe indeed it's a different one. This is the one I'm reading: https://en.delphipraxis.net/topic/6149-bestpractices-to-raise-or-not-to-raise-an-exception-in-a-class-constructor

 

Edited by Wagner Landgraf

Share this post


Link to post
1 hour ago, Rollo62 said:

I came already to the conclusion:

Raising on purpose in Create() is fine, especially to guard against missing or faulty injected parameters.

Amen to that.

Edited by Wagner Landgraf

Share this post


Link to post
49 minutes ago, Rollo62 said:

I would not need Guards on every internal operation, to ensure all parameters are OK.

 

So you are expecting and handling exceptions from the constructor but not expecting and not handling exceptions from any further method calls?

Interesting.

 

Share this post


Link to post
2 hours ago, Attila Kovacs said:

Can't see a damn question about memory management. Maybe I'm reading a different thread.

try...finally implies cleanup which in Delphi terms implies memory management :classic_smile:

 

Maybe memory management is not main topic, but it is certainly related.

Share this post


Link to post
32 minutes ago, Attila Kovacs said:

So you are expecting and handling exceptions from the constructor but not expecting and not handling exceptions from any further method calls?

Interesting.

The way I am reading that is: if you pass parameters in constructor to initialize fields and you validate them there, raising exception if they are not valid, then you can avoid checking those fields when you are using them in other places because you know they cannot hold invalid values.

  • Thanks 1

Share this post


Link to post
Just now, Dalija Prasnikar said:

The way I am reading that is: if you pass parameters in constructor to initialize fields and you validate them there, raising exception if they are not valid, then you can avoid checking those fields when you are using them in other places because you know they cannot hold invalid values.

Must be a very rare occasion where the parameter type itself does not cover the validity and the validity would not change with time, as far as I can think of. In this case of course a validation is not avoidable. However, I still think, that that is not the case behind the problem because it should be such rare, that he would not brought it up. I suspect over- and early-validation. At least, my advice was, not to do them if not neccessary.

Share this post


Link to post
14 minutes ago, Dalija Prasnikar said:

try...finally implies cleanup which in Delphi terms implies memory management

he has no problems with memory management, he even made it clear in just a couple of replies above so I have no idea what you trying to prove here

Share this post


Link to post
12 minutes ago, Attila Kovacs said:

Must be a very rare occasion where the parameter type itself does not cover the validity and the validity would not change with time, as far as I can think of. In this case of course a validation is not avoidable. However, I still think, that that is not the case behind the problem because it should be such rare, that he would not brought it up. I suspect over- and early-validation. At least, my advice was, not to do them if not neccessary.

A very common case is when you inject objects and interfaces in the constructor. At the very least you would want to check if they are not nil.

  • Like 3

Share this post


Link to post
9 minutes ago, Attila Kovacs said:

he has no problems with memory management, he even made it clear in just a couple of replies above so I have no idea what you trying to prove here

He was not the only one posting here. 

Share this post


Link to post
8 minutes ago, Attila Kovacs said:

he has no problems with memory management, he even made it clear in just a couple of replies above so I have no idea what you trying to prove here

I am talking about your comment to  @Wagner Landgraf where he mentioned memory management and you are saying this thread is not about memory management. 

 

I am not trying to prove anything, just wanted to say that memory management is indirectly involved in this conversation, and Wagner didn't post in the wrong thread.

  • Like 1

Share this post


Link to post
4 minutes ago, Wagner Landgraf said:

A very common case is when you inject objects and interfaces in the constructor. At the very least you would want to check if they are not nil.

I would eventually, if I'm using them in the constructor. Eventually, as I'm not sure that an exception("I'm dumb") is better than an AV.

If I'm not using them in the constructor, there is no reason to validate them.

Also, it can be still trash because it's not nil.

 

Share this post


Link to post
8 minutes ago, Wagner Landgraf said:

He was not the only one posting here. 

ok, you are right, I missed a lot from the beginning, my apologies

  • Thanks 1

Share this post


Link to post
8 minutes ago, Wagner Landgraf said:

At the very least you would want to check if they are not nil.

I don't really see the value of this, if the input is contracted to be assigned. That makes it a programming error, your tests will AV and you will fix it. No need to clutter the code with if not Assigned tests. 

Share this post


Link to post
1 minute ago, Attila Kovacs said:

I would eventually, if I'm using them in the constructor. Eventually, as I'm not sure that an exception("I'm dumb") is better than an AV.

If I'm not using them in the constructor, there is no reason to validate them.

Also, it can be still trash because it's not nil.

 

If the interface is required for your whole class to properly work, why you wouldn't want to check it in the constructor? Why allowing a instance to be created in an "invalid" state? You would have to check if the interface is nil everywhere in your code and for no use, since your class won't work without it.

Share this post


Link to post
1 minute ago, David Heffernan said:

I don't really see the value of this, if the input is contracted to be assigned. That makes it a programming error, your tests will AV and you will fix it. No need to clutter the code with if not Assigned tests. 

Fair enough, it was more an example to show that relying on the parameter type is sometimes not enough. But as a library writer I don't think it's bad to give a better error message to the user of my classes (whose code I have no control of) telling him what he did wrong, instead of raising a cryptic AV and waiting him to realize what he did wrong or even blame the framework itself.

  • Like 1

Share this post


Link to post
2 minutes ago, Wagner Landgraf said:

why you wouldn't want to check it in the constructor? 

Because it clutters the code and you can find such bugs other ways which don't clutter the code. 

Share this post


Link to post
1 minute ago, Wagner Landgraf said:

But as a library writer I don't think it's bad to give a better error message to the user of my classes (whose code I have no control of) telling him what he did wrong, instead of raising a cryptic AV and waiting him to realize what he did wrong or even blame the framework itself.

Oh for a library writer then yes, the calculations are different and there is much more justification for this

  • Like 1

Share this post


Link to post
17 hours ago, Darian Miller said:

Also consider passing items into a constructor rather than creating items within it to increase testability.  

Yep, what could be better than explicitly creating a canvas and icon for every created form 😄

 

Share this post


Link to post

I am a bit with @David Heffernan here: As long as I am the only one using the classes or even the team is, I tend to avoid checks for parameters or correct usage - despite documenting it in the comments (like XMLDOC) of course. For myself and the team I expect to take care to do it right, otherwise running the test suite will (should) surface that early enough.

 

In contrary, writing library code exposed to arbitrary users requires a bit more defensive programming, at least for the interface part the user is accessing directly.

 

Share this post


Link to post

Sliding into off-topic, but I avoid parameters to constructors. 

The primary reason is that parameterless constructors are a good fit for polymorphic constructs, as well as instantiations for generics. 
For configurations, I often give the instance an interface to fetch its own config from.  DI FTW.

 

  • Like 1

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×