Should I put a try-finally block after every Object.Create?

I have 开发者_如何转开发a general question about best practice in OO Delphi. Currently, I put try-finally blocks anywhere I create an object to free that object after usage (to avoid memory leaks). E.g.:

aObject := TObject.Create;
try
  aOBject.AProcedure();
  ...
finally
  aObject.Free;
end;

instead of:

aObject := TObject.Create;
aObject.AProcedure();
..
aObject.Free;

Do you think it is good practice, or too much overhead? And what about the performance?

It's definitely best practice to use try-finally.

In the event of an exception being raised, that object will be freed.

As for performance: measure before you optimise.

In my opinion, there is only one reason an objects construction should not be followed by (or "in" as Mason pointed out) a try / finally block.

1. If an objects' lifetime is being managed by another object.

This management can take three forms:

1. An object's reference has a scope beyond the local block and is freed elsewhere - like a field member freed in the destructor.
2. An object immediately added to a list which is in charge of freeing the object later.
3. An object with an associated lifetime manager, like how you pass the owner to a VCL control on construction.

With #1, when the reference has a broader scope, the reference should be set to nil immediately if it isn't constructed right away. That way when it is checked for reference you know you have an accurate reading. This is most common with member objects that are constructed as part of a larger class, and then cleaned up when the parent object is destroyed.

With #2, when an object is added to a list, you want to use a try-except block (one of the few times I use one) just in case an exception occurs after the object is constructed and before it is added to the managing list. Ideally the first line after the construction is adding it to the list, or the list is actually a factory class that gives you an object already added to the list.

With #3, when an object has another lifetime manager, you really should make sure having it managed by that manager is the right thing to do. If you are constructing a VCL control, you may be tempted to have the form (or any other control) own it, but that actually puts additional overhead on the construction and destruction. If possible you should explicitly free it, this is especially true if you put the control on once, then you know you will be freeing it in your form's destructor or when it closes. The only time you can't do this is if the control creation is more dynamic.

So yes, it is a best practice to use a lot of try / finally blocks. You should only have a few try / except blocks, and most all of them should trap very specific exception types, and / or re-raise the exception. If you have more try / except than try / finally blocks, then you are doing it wrong.

As Frank said, "as for performance: measure before you optimize." Repeating it to emphasize.

Also, if you're creating a bunch of objects in a method, you don't need to use a try..finally block for each of them. That can lead to an ugly indentation mess. create, try, create, try, create, try, do something, finally, free, finally, free, finally, free. Ugh! Instead, you can set the object references to nil at the top of the method, then create them all, do one try block, and free them all in the finally section.

That'll save some overhead and performance, (though you'll probably never notice the difference,) but more importantly it'll make your code cleaner and easier to read while maintaining the same level of safety.

To answer part two of your question:
try finally hardly has any overhead.

In fact there are plenty of methods having an implicit try...finally block. For instance just having a function using a local var of any Interface type and assigning that a value has.

--jeroen

Last year at Delphi Developer days I saw some of Marco Cantu's private stash of code and he calls try finally every single time he creates anything.

Somebody asked him about it, and he said he tries to do it all the time.

But it's especially a good idea for multi-threaded code when it comes to entering and exiting critical sections, although that's not on the topic, it's a good thing to remember.

Obviously sometimes it's a bit obtrusive and if it's not in the culture of your work environment to be spot-on in your robsutness it may make you look like a goodie-two-shoes. But I think it's a good idea. It's kind of like Delphi's attempt at enforced manual garbage collection.

If you're creating the object in a class's constructor and the object will be owned by the enclosing class, you'll want to free it in the owning class's destructor.

I tend to use FreeAndNil() instead of calling Free.

EDIT: But as others have said, you definitely always want to free what you create.

Yes it is always a good idea (essential) if the code that creates the object is responsible for free'ing it. If not, then try/finally isn't appropriate but then again neither is .Free in any case!

However, it can be cumbersome to have this boilerplate code peppering your "business logic", and you might want to consider an approach which has the same guarantee of freeing your objects but which is much cleaner (and has other benefits), such as my own AutoFree() implementation.

With AutoFree() your code could then be written:

aObject := TObject.Create;
AutoFree(@aObject);

aObject.AProcedure();

or alternatively, since the implementation uses a reference to a reference (to enable auto-NIL'ing as well as Free'ing), you can even pre-register your references that you wish to be AutoFree'd to keep such housekeeping declarations away from your business logic and keep the real "meat" of your code as clean as possible (this is especially beneficial when potentially multiple objects need to be Free'd):

AutoFree(@aObject1);
AutoFree(@aObject2);

aObject1 := TObject.Create;
aObject1.AProcedure();

// Later on aObject2 is (or may be) also created
 :

Not shown in my original posting is a later addition to the mechanism to support registering multiple references in a single AutoFree() call, but I'm sure you can figure out the changes needed to support this on your own, if you wish to be able to do this:

AutoFree([@aObject1, @aObject2]);

aObject1 := TObject.Create;
aObject1.AProcedure();

// Later on aObject2 is (or may be) also created
 :

Even if it is much recommended to do that, I won't always do it.

My rules of using or not using the try/finally:

• The chance for that object to crash and burn.
• Number of times the object is created. If I know that your object is created rarely (not more than 5-6 time in application life time) I can live with a 20KB memory leak - in case it 'dies' without being freed.
• Amount of leaked memory in case the object crashes.
• Code complexity. Try/except is making the code to look really ugly. If there is a 5 lines procedure, I always use try/except.
• Application file span. If your application needs to run for days, then I definitively want to free ANY piece of memory else the leak will accumulate.

The only place where is difficult to make a decision is when you need performance while creating the object thousands of times (in a loop for example). In this case, I don't use try/except if the object is performing simple tasks and there is a small chance to see it crashing.