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

[Rollo62 536]

Hi there,

 

I have a general question regarding this topic.

There is a lot information out there, even in the DocWiki.

https://blogs.embarcadero.com/what-are-the-mistakes-we-made-in-creating-delphi-objects/

https://sergworks.wordpress.com/2015/01/21/think-twice-before-raising-exception-in-constructor/

https://stackoverflow.com/questions/39110021/delphi-raise-exception-in-constructor

 

To summarize:

It is perfectly fine to raise an exception in a class constructor ( and in some cases its even not avoidable, I assume ).

 

My question is: Shall and raise an exception on purpose, and if so, what strategy is best ?

 

In a normal situation, this is completely OK.

LA := TRaiseOnMinus.Create( -2 );  		//<== Raise
try
    //some code
finally
    LA.Free; 					//<== Never called
end;

But like that it causes issues:

try
    LA := TRaiseOnMinus.Create( -2 );	//<== Raise
    //some code
finally
    LA.Free;   //<== Will be called on an unconstructed object
end;

 

But there is the pattern of encapsuling multiple try - finally's

How shall I handle that ?

LA := nil;
LB := nil;
try
    LA := TRaiseOnMinus.Create( AInputA );	//<== PossibleRaise
    try
        LB := TRaiseOnMinus.Create( AInputB );	//<== Possible Raise
    finally
        LB.Free; 
    end;

finally
    LA.Free; 
end;

Either I can guarantee that NO Exception can be raised ( can I guarantee that really ) ?

Or maybe guarding the Free might help, but I doubt that.

LA := nil;
LB := nil;
try
    LA := TRaiseOnMinus.Create( AInputA );	//<== PossibleRaise
    try
        LB := TRaiseOnMinus.Create( AInputB );	//<== Possible Raise
    finally
        if Assigned( LB ) then
            LB.Free; 
    end;

finally
    if Assigned( LA ) then
        LA.Free; 
end;

Are there any recommendations or best practices, about using Exceptions in class constructors ( Shall I use them, or not ) ?

 

I see the problem that the caller never knows if Create might be able to throw an exeption, so he must be analysing the internals of the TRaisedOnMinus class first.

 

Beside that, there could be the possibility to prevent any damage from inside the class, by guarding internals, to ensure that double-calling Free doesn't do any damage.

Only I'm afraid that this guarding inside Destroy is bad practice too ( but I like double-safetly measures anyway ).

constructor TRaiseOnMinus.Create( AParam : Integer );
begin
    inherited;
    FMyList := nil;

    if AParam < 0 then
        raise Exception.Create( 'Bum' );
                 
    FMyList := TStringList.Create;                 
end;

destructor TRaiseOnMinus.Destroy;
begin
    if Assigned( FMyList ) then
        FMyList.Free;

    inherited;
end;

How do you design classes with exceptions ?

 

Edited January 11, 2022 by Rollo62

[Lajos Juhász 293]

You can use multiple try finally or>

LA := nil;
LB := nil;
try
    LA := TRaiseOnMinus.Create( AInputA );	//<== PossibleRaise
    LB := TRaiseOnMinus.Create( AInputB );	//<== Possible Raise
finally
  LA.Free;
  LB.Free;
end;

In this case before try LA and LB are both nil in case a constructor raises an exception it will still be NIL thus Free will do nothing.

[Lajos Juhász 293]

You missed the example with nested try finally. It should be:

 

    LA := TRaiseOnMinus.Create( AInputA );	//<== PossibleRaise
    try
        LB := TRaiseOnMinus.Create( AInputB );	//<== Possible Raise
        try
        finally
          LB.Free;
        end;
    finally
        LA.Free; 
    end;

 

[Uwe Raabe 2057]

20 minutes ago, Rollo62 said:

It is perfectly fine to raise an exception in a class constructor ( and in some cases its even not avoidable, I assume ).

Although that might be the case, I recently turned to form a habit that avoids anything causing exceptions inside constructors. That may also lead to avoid giving parameters to constructors - at least when they are not being handled seamlessly during the lifetime of the instance. That allows to concentrate on the real work to be done instead of figuring out some tricky handling of edge cases.

 

  LA := TRaiseOnMinus.Create;
  try
    LA.Initialize(AInputA);  //<== PossibleRaise
    LB := TRaiseOnMinus.Create;
    try
      LB.Initialize(AInputB);  //<== Possible Raise
      { do the normal stuff with LA and LB }
    finally
      LB.Free;
    end;
  finally
    LA.Free; 
  end;

 

[Dalija Prasnikar 1396]

 

Some bullet points:

 

• if Assigned(Obj) then Obj.Free is redundant, you can just call Obj.Free - Free is static and can be called on nil instance and also Free has nil check within before calling destructor
• constructors are allowed to raise exceptions
• destructors must never raise exceptions (without handling them within with try...except)
• destructors can be called on half constructed instance (if the constructor fails, destructor chain will be automatically called) and must be able to handle such scenarios without causing trouble - raising exceptions
• when constructor fails assignment to variable will never happen
• local object variables are never automatically initialized and they can contain random garbage
• object instance fields are always zero initialized when constructor chain is being called

 

Because of the above correct construction pattern is

 

var
  LA: TRaiseOnMinus;
begin
  LA := TRaiseOnMinus.Create( -2 );  		
  try
    //some code
  finally
    LA.Free; 					
  end;
end;

 

With multiple constructors there are multiple options, but I prefer using only single try...finally because destructors must not raise exceptions, single try finally is faster and cleaner

 

var
  LA, LB: TRaiseOnMinus;
begin
  LB := nil;
  LA := TRaiseOnMinus.Create( -2 );  		
  try
    LB := TRaiseOnMinus.Create( -3 );                                                   
    //some code
  finally
    LB.Free;
    LA.Free; 					
  end;
end;

 

Correct constructor and destructor for your class would be:

 

constructor TRaiseOnMinus.Create( AParam : Integer );
begin
    inherited;
    if AParam < 0 then
        raise Exception.Create( 'Bum' );                 
    FMyList := TStringList.Create;                 
end;

destructor TRaiseOnMinus.Destroy;
begin
  FMyList.Free;
  inherited;
end;

 

Only if you need to do some additional work with FMyList in destructor, then you must check it for nil before using it. In such cases you can find call to Free within the same check, but not because it wouldn't work outside.

 

destructor TRaiseOnMinus.Destroy;
begin
    if Assigned(FMyList) then
      begin
        FMyList.DoSomethingImportant;
        FMyList.Free;
      end;
    inherited;
end;

 

 

[Lars Fosdal 1792]

I prefer NOT to raise exceptions in constructors - class nor otherwise.

It makes securing the build up of the instance hierarchy too complicated. 

A lack of connectivity or functionality should only be addressed at the time of use, not at the time of instancing, considering their absence may be temporary.

 

There is one case though - during testing, assertions may be used to check for stuff that SHOULD be in place.

[Fr0sT.Brutal 900]

Any c-tor could raise an exception if there's not enough memory. So IMHO scheme

foo := TFoo.Create;
try
...
finally
  FreeAndNil(foo);
end

is OK. Otherwise you'll get "Variable might be uninit-ed" warning. Of course you can wrap this clause in external try-catch to handle an exception internally (default behavior is call Application.HandleException method)

Regarding the ability to raise itself, yes, there's no helpers in the language. Sometimes I dream of something like Java's "throws" clause.

[Rollo62 536]

3 hours ago, Uwe Raabe said:

  LA := TRaiseOnMinus.Create;
  try
    LA.Initialize(AInputA);  //<== PossibleRaise
    ...

 

@Uwe Raabe

Thanks Uwe, it turns out that I used a similar approach for a few classes.

Only that I called them Conf_Setup_...,  which allows me to have different Conf_Setup_.... for different purposes   ( Conf_Setup_Initial,  Conf_Setup_Module, Conf_Setup_Events, ....  ).

This way I have them all nicely bundled together under the same Prefix.

  LA := TRaiseOnMinus.Create;
  try
    LA.Conf_Setup_Initial( AInputA );  //<== PossibleRaise
    ...

This works Ok, but it feels a little wrong, when I try to use clean DI via constructor injection a the same time.

To separate this, from constructor to property injection, would bring DI to a less strict level, IMHO.

I have hope to keep simple classes as clean as possible, that was the main reason of my initial question in the first place.

 

Would clean separation of construction and initialization be able to resolve ALL the unwanted issues completely ?

If that is the case, then I think the overhead is worth it.

Only I am not so sure if I can achive the same, with lower overhead cost too.

 

2 hours ago, Dalija Prasnikar said:

 

Some bullet points:

 

• if Assigned(Obj) then Obj.Free is redundant, you can just call Obj.Free - Free is static and can be called on nil instance and also Free has nil check within before calling destructor
• constructors are allowed to raise exceptions 
• destructors must never raise exceptions (without handling them within with try...except)
• destructors can be called on half constructed instance (if the constructor fails, destructor chain will be automatically called) and must be able to handle such scenarios without causing trouble - raising exceptions
• when constructor fails assignment to external variable will never happen
• local object variables are never automatically initialized and they can contain random garbage
• object instance fields are always zero initialized when constructor chain is being called

@Dalija Prasnikar

Thanks, these rules are very helpful to sort out the right behaviour.

2 hours ago, Dalija Prasnikar said:

Correct constructor and destructor for your class would be:

constructor TRaiseOnMinus.Create( AParam : Integer );
begin
    inherited;
    if AParam < 0 then
        raise Exception.Create( 'Bum' );                 
    FMyList := TStringList.Create;                 
end;

destructor TRaiseOnMinus.Destroy;
begin
  FMyList.Free;
  inherited;
end;

@Dalija Prasnikar

Doesn't that bite with your bullet point from above ?

Once you have half-created instance, you also may have hals constructed internal fields, or is that guaranteed that this never happened ?

Thats why I put the FMyList := nil; before the Raise, but it's more like an old habit of mine, to initialize before use.

Ok, with your rule:   "object instance fields are always zero initialized when constructor chain is being called", thats obsolete.

But are all kind of field types secured, like Pointers, [ Weak ] field, and whatsnot, ... ?

My habit shall prevent any unexpected glitch, but is of course double-safety in most cases.

(Is this an AntiPattern maybe ? )

 

I also used the "flat" try - finally here and there, but this always left a bad taste to me.

What happend if I use this for more than two variables, which often may happen in Bitmap related stuff ?

var
  LA, LB, LC: TRaiseOnMinus;
begin
  LC := nil;
  LB := nil;
  LA := nil;
  try
    LA := TRaiseOnMinus.Create( -2 );  		
    LB := TRaiseOnMinus.Create( -3 );                                                   
    LC := TRaiseOnMinus.Create( -3 );                                                   
    //some code
  finally
    LC.Free;
    LB.Free;
    LA.Free; 					
  end;
end;

Wouldn't it be OK as well, when I use them "flat", but take special care on managing them in the right order ( ABC - CBA ), like above ?

Why should I use one Create outside the try scope ?

 

 

2 hours ago, Lars Fosdal said:

I prefer NOT to raise exceptions in constructors - class nor otherwise.

It makes securing the build up of the instance hierarchy too complicated. 

A lack of connectivity or functionality should only be addressed at the time of use, not at the time of instancing, considering their absence may be temporary.

 

There is one case though - during testing, assertions may be used to check for stuff that SHOULD be in place.

@Lars Fosdal

Right, I also avoid to raise Exceptions, but currently I am working on a class where this could make sense.

So I would like to find the best answer or rule-of-thumb for this case, could be like this:

- never raise Exception on purpose, it's evil

- never raise Exception on purpose, it's an antipattern ( because ... see GOF page xxx )

- you can raise Exceptions safely, if you do prepare this and that

- if you always do it like this, you guaranteed to never ever run into any issues.

 

I think we already have a lot of useful rules identified already, thanks to all.

 

1 hour ago, Fr0sT.Brutal said:

Any c-tor could raise an exception if there's not enough memory. So IMHO scheme

@Fr0sT.Brutal

Yes, I only try to find the rules for raising my own Exceptions on purpose.

But they may raise by system at any time, maybe not only memory, but also file system or other hardware related.

Would be good to have the best set of rules for all cases, my exceptions and system exceptions, in the same way.

 

 

 

 

Edited January 11, 2022 by Rollo62

[David Heffernan 2345]

This is pertinent to some of the discussion here: https://stackoverflow.com/a/8550628/505088

[Rollo62 536]

1 hour ago, David Heffernan said:

This is pertinent to some of the discussion here: https://stackoverflow.com/a/8550628/505088

Thanks for those descriptions, they are very enlightning too.

 

I use the Assigned() check as another (bad) habit too, inspired by the DUnit-Tests, which is somewhat related to the separation idea from above ...

destructor TRaiseOnMinus.Destroy;
begin
    if Assigned(FMyList) then
      begin
        FMyList.Conf_Teardown;
        FMyList.Free;
      end;
    inherited;
end;

Related to the Conf_Setup_.... scheme,  I like to implement Conf_Teardown as well, but this is maybe less reasonable.

My goal with that approach was to get also all cleanup in one place, outside the Destroy, thats why I often use the Assigned() before .Free.

 

But as explained above, I find Conf_Setup_... and Conf_Teardown not that attractive, and hope to get rid of this.

 

One reason ist that the external call to Conf_Setup is maybe missing, and the internal fields stay maybe uninitialized.

procedure TTest.Run;
var  LA : TRaiseOnMinus;
begin
    LA := nil;
    try
        LA := TRaiseOnMinus.Create;  //<= No more exception
        LA.Conf_Setup( -1 );         //<= It's possible here now
        LA.Conf_Setup( TStringList.Create );         //<= Another configuration, maybe this line is missing ??

    finally
        if Assigned( LA ) than
        begin
            LA.Conf_Teardown;
            LA.Free;
        end;
    end;
end;

Either I guard any internal access to the internal StringList field variable, or I throw an exception at such accesses.

This behaviour I dislike very much in the Conf_Setup approach, and I see no simple way how to intrinsically detect if Conf_setup was called, or not.

Allowing uninitialized, unguarded fields would cause random AV, which is by far the worst case of all, IMHO.

 

 

 

Edited January 11, 2022 by Rollo62

[David Heffernan 2345]

In a destructor you do need to test Assigned on members if you call any method other than Free. That said, destructor must not raise exceptions. And its generally better to move the call to those teardown methods into the destructor of the class. 

[Rollo62 536]

9 minutes ago, David Heffernan said:

In a destructor you do need to test Assigned on members if you call any method other than Free. That said, destructor must not raise exceptions. And its generally better to move the call to those teardown methods into the destructor of the class. 

Right, Destroy should have no exceptions.

But is there any difference if the Exception happen in Destroy or in Conf_Teardown ?

If Conf_Teardown is called only in Destroy, and raises an exception, the bad result might be the same.

 

The only advantage to put things in Teardown might, that there a  try - finally block could catch such Events, in a central manner.

Which is maybe a better place for try-finally, than to put that in Destroy itself.

 

Edited January 11, 2022 by Rollo62

[David Heffernan 2345]

The advantage of moving that teardown  call to the subject's destructor is that you don't need to test Assigned. If it raises an exception then you are still hosed.

[Dalija Prasnikar 1396]

3 hours ago, Rollo62 said:

But are all kind of field types secured, like Pointers, [ Weak ] field, and whatsnot, ... ?

Yes. All fields are zero initialized.

3 hours ago, Rollo62 said:

My habit shall prevent any unexpected glitch, but is of course double-safety in most cases.

(Is this an AntiPattern maybe ? )

There is no double safety if something cannot ever happen. If you have extra initialization code then such unnecessary code can obscure other important code. Keep it simple. If you are not sure what is initialized and when, it is better to check and learn than to use code just in case.

 

3 hours ago, Rollo62 said:

I also used the "flat" try - finally here and there, but this always left a bad taste to me.

What happend if I use this for more than two variables, which often may happen in Bitmap related stuff ?

var
  LA, LB, LC: TRaiseOnMinus;
begin
  LC := nil;
  LB := nil;
  LA := nil;
  try
    LA := TRaiseOnMinus.Create( -2 );  		
    LB := TRaiseOnMinus.Create( -3 );                                                   
    LC := TRaiseOnMinus.Create( -3 );                                                   
    //some code
  finally
    LC.Free;
    LB.Free;
    LA.Free; 					
  end;
end;

Wouldn't it be OK as well, when I use them "flat", but take special care on managing them in the right order ( ABC - CBA ), like above ?

It is fine to construct as many instances as you need and protect them with single try...finally. You don't need to pay attention to the order of destruction, unless there is some special reason where one instance still holds reference to other one you are destroying sooner, where you might have problem with dangling pointers. 

3 hours ago, Rollo62 said:

Why should I use one Create outside the try scope ?

There is no functional difference, only one nil assignment more in your case - if you feel like you can more easily follow the code if multiple instances are all initialized before try..finally, that is fine.

 

Remember, try...finally is used just for cleanup in case of exception, not for handling the exception. If exception is raised inside try..finally it will be propagated to the next exception handler block - try...except just the same it would if it is raised outside try..finally.

 

Again, if there is exception raised in constructor, assignment to the variable will never happen. So if LA is nil and the you call LA := TRaiseOnMinus.Create(-2) in finally block LA will still be nil. Automatic cleanup for that unassigned object instance is happening in hidden code inserted by compiler. If the instance is successfully created, then automatic cleanup will not happen.

3 hours ago, Rollo62 said:

So I would like to find the best answer or rule-of-thumb for this case, could be like this:

- never raise Exception on purpose, it's evil

- never raise Exception on purpose, it's an antipattern ( because ... see GOF page xxx )

- you can raise Exceptions safely, if you do prepare this and that

- if you always do it like this, you guaranteed to never ever run into any issues.

Raising exceptions in constructor is not evil nor bad practice. It is perfectly fine thing to do.

 

Whether you will raise exception in constructor or not, is matter of each particular class functionality. If you pass invalid parameter to a constructor it makes sense that such constructor raises exception, because otherwise your object is not properly functional instance, If you don't raise exception then, you would have to sprinkle your code all over inside that class and where you use it with checks for that particular invalid condition. When you have some condition that causes failure failing soon is always better than failing later on. If you have some reasons not to raise exception or there is nothing special that would require raising exception that is also fine. Constructing objects can always raise OutOfMemory so you always need to be prepared to do the cleanup anyway.

 

Take for instance TFileStream - if constructor fails to open or create file it will raise an exception. And you can handle that failure at that point. If you don't have to handle it at that point, that means you have opened file too soon. 

[Attila Kovacs 629]

I can't think of everything right now but the 

LA := TRaiseOnMinus.Create( -2 );	//<== Raise

seems to me a double act. Instantiate an object and doing something which raises an exception because of the parameter.

In the constructor I mostly store the argument into a class field so It will cause an exception later, thus the constructor is not affected.

Similar to Uwe's solution, but without initialize, just let it fail somewhere, but not in the constructor.

 

 

Also, I suspect that you are using in this example exceptions to control the app.

Don't do that if you can. Make the parameter type unsigned or let it fail somewhere if the parameter is negative but do not check it in the constructor and call raise.

(If you can)

[Dmitriy M 10]

30 minutes ago, Attila Kovacs said:

In the constructor I mostly store the argument into a class field so It will cause an exception later, thus the constructor is not affected.

 

Similar to Uwe's solution, but without initialize, just let it fail somewhere, but not in the constructor.

The problem with this pattern is that most methods of the class now has to perform some kind of validation before doing their job.

[Attila Kovacs 629]

11 minutes ago, Dmitriy M said:

The problem with this pattern is that most methods of the class now has to perform some kind of validation before doing their job.

If those methods are time critical you can do a lazy initialization or validate outside or even avoid OOP or whatever, but I would not validate in the constructor if I don't have to.

Edited January 11, 2022 by Attila Kovacs

[David Heffernan 2345]

11 minutes ago, Attila Kovacs said:

If those methods are time critical you can do a lazy initialization or validate outside or even avoid OOP or whatever, but I would not validate in the constructor if I don't have to.

What do you think about TFileStream.Create raising an exception if the file can be opened in the requested mode for whatever reason?

 

I cannot see the problem with this, or indeed any constructor raising. If you are going to admit exceptions in your program at all, you must be resilient to them. Why is it any different to get the exception raised in a constructor than anywhere else. 

Edited January 11, 2022 by David Heffernan

[Dalija Prasnikar 1396]

11 minutes ago, Attila Kovacs said:

If those methods are time critical you can do a lazy initialization or validate outside or even avoid OOP or whatever, but I would not validate in the constructor if I don't have to.

Nobody said that you must validate in the constructor. It all depends whether this is critical or not and at which point.

 

There are no wrong or right answers here, it all depends how class functionality and how it is used. Each approach has downsides and upsides.

[Attila Kovacs 629]

4 minutes ago, David Heffernan said:

What do you think about TFileStream.Create raising an exception if the file can be opened in the requested mode for whatever reason?

 

Nothing. It's the class' design pattern. There could be an extra "Open" where the exception raises, makes no difference for me.

But, following a design pattern makes life easier, so I'm trying to follow some rules, and yes, this can make things easier "better". It does not always work though.

[David Heffernan 2345]

43 minutes ago, Attila Kovacs said:

Nothing. It's the class' design pattern. There could be an extra "Open" where the exception raises, makes no difference for me.

But, following a design pattern makes life easier, so I'm trying to follow some rules, and yes, this can make things easier "better". It does not always work though.

I don't see the point of trying not to raise in a constructor as a design goal. Raise the exception at the point where you know it needs to be raised. 

[Attila Kovacs 629]

33 minutes ago, David Heffernan said:

Raise the exception at the point where you know it needs to be raised. 

I'm always rising them there where it's not needed and mostly when I don't know. Sometimes not. You should have come up earlier with that.

[Rollo62 536]

11 hours ago, Dalija Prasnikar said:

...  Raising exceptions in constructor is not evil nor bad practice. It is perfectly fine thing to do. ...

Right, but why does it feel so bad

 

Maybe I need to think a little out-of-the-box:

1. If I have a little complex classes with quite a few parameters that may rearranged in many ways, while the class is intendet to use direkt operations anyway,

   then the Conf_Setup pattern for initialization seems to be reasonable.

   Conf_Setup can throw exceptions when needed.

   If I remove all the guards on each operation, this is a little risky step.

   When "forgetting" such Setup, might result in failures usually that should be directly "visible" and easily spotable, but they also might occur in any unpleasant, unforseen situations.

   So I better use Conf_Setup ALWAYS with Guards on each operation.

 

2. If I have small, simple objects with < 5 parameters, which are mainly intended to be passed to other objects ( like TTtreeNode ), then the raise in Create might be the right choice.

  As advantage I can remove all Guards inside that class.

 

3. A third rule might be, if you have a class of 1.) it is maybe too complex.  Try  to re-arrange or decompose it, so that the class(es) can fall under 2.)

    But this is not so easy sometimes, as you might know.

    This would mean the Conf_Setup rule 1.) is something temporarily only, until I find some better design solution.

 

So a set of rules for different use cases should be fine too, but I'm not sure if these considerations are complete yet.

But looking at 1-3.), I would think that the final goal could be to move most classes to 2.), with raising exceptions at the point of their creation.

So reaching design 2.) should be the most attractive, I would assume.

 

 

Edited January 12, 2022 by Rollo62

[Fr0sT.Brutal 900]

11 hours ago, Attila Kovacs said:

In the constructor I mostly store the argument into a class field so It will cause an exception later, thus the constructor is not affected.

Similar to Uwe's solution, but without initialize, just let it fail somewhere, but not in the constructor.

Generally, I don't like this approach. It moves rejection point to some another (probably random) place, requires validation in every method and allows invalid values to exist between creation and validation. However, there could be cases where this way is the most logical. F.ex., I have file writer class that could utilize multiple files during its lifetime because of rotation or file pattern change. So c-tor only sets basic properties and Open method opens a file, probably raising an exception on failure.

[David Heffernan 2345]

Again, can somebody explain what problem is caused by raising in a constructor. We all seem fine with TFileStream doing this, so all I can see is dogma.