Jump to content
Rollo62

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

Recommended Posts

If you construct something complex that allocates memory in multiple sub-objects or blocks, how do you prevent it from leaking if an exception is raised during construction? By having exception handling in the constructors?

Share this post


Link to post
2 minutes ago, Lars Fosdal said:

If you construct something complex that allocates memory in multiple sub-objects or blocks, how do you prevent it from leaking if an exception is raised during construction? By having exception handling in the constructors?

No, by having a destructor that can handle partly constructed objects.

  • Like 3

Share this post


Link to post
13 minutes ago, David Heffernan said:

Again, can somebody explain what problem is caused by raising in a constructor

The only problem with it is that you've to write your code in form:

 

lObj:=TMyClass.Create(....);
try
.....
finally
  lObj.Free;
end;

Anyway you would write it in this form. So there is no problem. I believe it's just another phobia. People can be afraid from correct coding style but will happily use FreeAndNil everywhere in the code.

Share this post


Link to post

Which means that you must add exception handling in all levels of the destructor chain?

destructor TMyThing.Destroy;
begin
  try
    inherited;
  finally
    try 
      // destroy my things
    except
      // this was unfortunate
    end
  end;
end;

 

Share this post


Link to post
3 minutes ago, Lars Fosdal said:

Which means that you must add exception handling in all levels of the destructor chain?

No. Its pointless trying to handle exceptions in destructors. If that happens the process should terminate. 

Share this post


Link to post
2 minutes ago, Lars Fosdal said:

Which means that you must add exception handling in all levels of the destructor chain?


destructor TMyThing.Destroy;
begin
  try
    inherited;
  finally
    try 
      // destroy my things
    except
      // this was unfortunate
    end
  end;
end;

 

No, you don't.

 

If the instance is never constructed it will be nil in the destructor and you can just call Free on such instance. You only need to consider that any instance in the destructor can be nil, so if you are calling any other methods you need to check for nil before calling those in destructor.

 

  • Like 1

Share this post


Link to post

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

var MyThing := TThing.Create;
try
  MyThing.DoIt;
finally
  MyThing.Free;
end;

 

Share this post


Link to post
5 minutes ago, Lajos Juhász said:

The only problem with it is that you've to write your code in form:

 

lObj:=TMyClass.Create(....);
try
.....
finally
  lObj.Free;
end;

Anyway you would write it in this form. So there is no problem. I believe it's just another phobia. People can be afraid from correct coding style but will happily use FreeAndNil everywhere in the code.

No. The try finally here protects the lifetime management of the instantiated object, after the constructor has completed. An exception in the constructor will bubble up from here and never enter the try. This is a very very common misunderstanding. 

Share this post


Link to post
Just now, Lars Fosdal said:

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

Don't you know that exceptions bubble up the call stack? If you try to handle them immediately then you've just got crappy error code type code. The entire point of exceptions is that you don't need to deal with them at the point where the error occurs. 

Share this post


Link to post
4 minutes ago, Lars Fosdal said:

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?


var MyThing := TThing.Create;
try
  MyThing.DoIt;
finally
  MyThing.Free;
end;

 

Actual purpose of try...finally in above code is to ensure cleanup of the MyThing instance if MyThing.DoIt raises exception. If constructor raises the exception there will be automatic cleanup and MyThing.Free will never be, nor it should be called.

Edited by Dalija Prasnikar

Share this post


Link to post

So, basically

try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

Share this post


Link to post
16 minutes ago, David Heffernan said:

No. The try finally here protects the lifetime management of the instantiated object, after the constructor has completed. An exception in the constructor will bubble up from here and never enter the try. This is a very very common misunderstanding. 

Exactly what I was saying.

In case you'write:

 

try

  lObj:=TMyClass.create;

finally

  lObj.Free;

end;

If the constructor raises the exception the RTL will clear that for you. However in case lObj as the name suggest is a local variable and you didn't assigned nil to it the value will be undefined and calling free on that reference can cause AV (in case you're not lucky that the value was nil).

Edited by Lajos Juhász
more information.

Share this post


Link to post
5 minutes ago, Lajos Juhász said:

Exactly what I was saying.

You described the correct construction pattern as a "problem" which is what confused me. 

Share this post


Link to post
1 hour ago, Rollo62 said:

Right, but why does it feel so bad :classic_huh:

You tell me. It doesn't feel bad to me at all :classic_smile:

 

I think you are overcomplicating. 

 

If you have class that requires some setup (that will commonly not be changed once object is created) to function properly then passing those parameters through constructor and raising exception in constructor is the best and simplest thing to do.

 

Use the approach that requires the least amount of code for safely using such class and where you cannot easily forget to do something vital. Usually that will be the best code.

Share this post


Link to post

 

1 hour ago, Fr0sT.Brutal said:

requires validation in every method

what if time is a factor on that calls? are you swearing while you writing the code?;)

 

Share this post


Link to post
21 minutes ago, Lars Fosdal said:

So, basically


try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

If you want to handle the exception on site, then the second option is the wrong way to do it for several reasons. 

 

Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so. If you are fine to bubble up that exception further on and if except part (handling the exception) cannot raise exceptions, you don't need try..finally at all.

  • Like 2

Share this post


Link to post
3 minutes ago, Dalija Prasnikar said:

Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so.

Actually I only handle exceptions where I have a plan B, otherwise I could only translate the messages to other languages.

Share this post


Link to post
30 minutes ago, Lars Fosdal said:

So, basically


try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

The whole point of exceptions is that in a huge number of cases it is impossible to "deal with it" at the point where the error is raised. The code has been called by another function that excepts it to succeed. If it can fail, but return normally without an exception bubbling up, then you need to return information to the caller that the function has failed. And the caller may in turn be have its own caller that needs to know this. And we are right back to exception free code where errors are indicated by boolean or integer error code return values.

 

Exceptions that bubble up the stack are only difficult to manage if you don't write your exception raising and handling code properly. For sure it is perfectly possible to write crappy exception code. But you are pointing the finger in the wrong place. 

  • Like 4

Share this post


Link to post
1 minute ago, Attila Kovacs said:

Actually I only handle exceptions where I have a plan B, otherwise I could only translate the messages to other languages.

What do you mean by plan B?

Share this post


Link to post
2 minutes ago, Attila Kovacs said:

@Dalija Prasnikar Hm, I really don't know what would I do on out of memory, never had to deal with that.

Depends on the context.

If you open some file for processing and it turns out to be a too large so processing it raises out of memory, you just clean up and tell to the user: "Sorry, your file is too large and you don't have enough memory." But after proper cleanup your application will be in fine condition to process some other smaller file. 

On the other hand, there are situations where out of memory is not recoverable. You still may be able to show message to the user there is not enough memory, but you will just have to kill the application after that.

  • Like 2
  • Thanks 1

Share this post


Link to post
56 minutes ago, Attila Kovacs said:

 

what if time is a factor on that calls? are you swearing while you writing the code?;)

 

If time is a factor, these methods surely must avoid checks of init state. Yep, sometimes I'm swearing but mostly dealing with somebody's code; I'm pretty sure I'll be swearing on classes designed with your approach 😛

For me: c-tor arguments should contain crucial values that need to be set and perform some init actions so that other methods are sure the things are ready for work and they won't have to check stuff every time. You can't forget to call c-tor but you easily can forget to call separate .Init method.

  • Like 2
  • Haha 1

Share this post


Link to post

I was curious so I made this useless statistics over one of my projects

 

WSL (always backup your files before playing with scripts on them)

 

for i in `find . -name "*.pas" -type f`; do cat $i | sed -nr '/\bconstructor\b/I{:a;N;/end\;/I!ba;/begin/Ip}' | sed -e '1i\'$i'' | grep constructor; done | wc -l;

250 constructors (180 with parameter)

for i in `find . -name "*.pas" -type f`; do cat $i | sed -nr '/\bconstructor\b/I{:a;N;/end\;/I!ba;/\sraise/Ip}' | sed -e '1i\'$i'' | grep constructor; done | wc -l;

11 explicit call for raise in a constructor (with parameter)

 

if my scripts are right

 

btw. Delphi sources 5137 / 306  (3937/244 with parameter)

Edited by Attila Kovacs

Share this post


Link to post

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

 

Foo := TFoo.Create;
try
finally
  Foo.Free;
end;

And let the exception raising begin. Anywhere.

  • Like 4

Share this post


Link to post

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

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

×