Best practice to do nested TRY / FINALLY statement

Question

Hi What is the best way to do nested try & finally statements in delphi?

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    cds3  : TClientDataSet;
    cds4  : TClientDataSet;
begin
  cds1      := TClientDataSet.Create(application );
  try
    cds2      := TClientDataSet.Create(application );
    try
      cds3      := TClientDataSet.Create(application );
      try
        cds4      := TClientDataSet.Create(application );
        try
        ///////////////////////////////////////////////////////////////////////
        ///      DO WHAT NEEDS TO BE DONE
        ///////////////////////////////////////////////////////////////////////
        finally
          cds4.free;
        end;

      finally
        cds3.free;
      end;
    finally
      cds2.free;
    end;
  finally
    cds1.free;
  end;
end;

Can you Suggest a better way of doing this?

Answer 1

how about the following:

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    cds3  : TClientDataSet;
    cds4  : TClientDataSet;
begin
  cds1      := Nil;
  cds2      := Nil;
  cds3      := Nil;
  cds4      := Nil;
  try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);
    cds3      := TClientDataSet.Create(nil);
    cds4      := TClientDataSet.Create(nil);
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    freeandnil(cds4);
    freeandnil(cds3);
    freeandnil(cds2);
    freeandnil(Cds1);
  end;
end;

This keeps it compact, and only attempts to free the instances which were created. There really is no need to perform the nesting since ANY failure will result in dropping to the finally and performing all of the cleanup in the example you provided.

Personally I try not to nest within the same method... with the exception being a try/try/except/finally scenario. If I find myself needing to nest, then to me that is a great time to think refactoring into another method call.

EDIT Cleaned up a bit thanks to the comments by mghie and utku.

EDIT changed the object creation to not reference application, as its not necessary in this example.

Answer 2

I'd use something like this:

var
  Safe: IObjectSafe;
  cds1 : TClientDataSet;
  cds2 : TClientDataSet;
  cds3 : TClientDataSet;
  cds4 : TClientDataSet;
begin
  Safe := ObjectSafe;
  cds1 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds2 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds3 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  cds4 := Safe.Guard(TClientDataSet.Create(nil)) as TClientDataSet;
  ///////////////////////////////////////////////////////////////////////
  ///      DO WHAT NEEDS TO BE DONE
  ///////////////////////////////////////////////////////////////////////

  // if Safe goes out of scope it will be freed and in turn free all guarded objects
end;

For the implementation of the interface see this article, but you can easily create something similar yourself.

EDIT:

I just noticed that in the linked article Guard() is a procedure. In my own code I have overloaded Guard() functions that return TObject, above sample code assumes something similar. Of course with generics much better code is now possible...

EDIT 2:

If you wonder why try ... finally is completely removed in my code: It's impossible to remove the nested blocks without introducing the possibility of memory leaks (when destructors raise exceptions) or access violations. Therefore it's best to use a helper class, and let the reference counting of interfaces take over completely. The helper class can free all objects it guards, even if some of the destructors raise exceptions.

Answer 3

There's another variation of the code without nested try ... finally that just occurred to me. If you don't create the components with the AOwner parameter of the constructor set to nil, then you can simply make use of the lifetime management that the VCL gives you for free:

var
  cds1: TClientDataSet;
  cds2: TClientDataSet;
  cds3: TClientDataSet;
  cds4: TClientDataSet;
begin
  cds1 := TClientDataSet.Create(nil);
  try
    // let cds1 own the other components so they need not be freed manually
    cds2 := TClientDataSet.Create(cds1);
    cds3 := TClientDataSet.Create(cds1);
    cds4 := TClientDataSet.Create(cds1);

    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////

  finally
    cds1.Free;
  end;
end;

I'm a big believer in small code (if it's not too obfuscated).

Answer 4

If you want to go this (IMO) ugly route (group handling with initialization to nil to know if freeing is needed), you at least MUST guarantee that you won't let an exception in one of the destructor prevent from freeing the rest of your objects.
Something like:

function SafeFreeAndNil(AnObject: TObject): Boolean;
begin
  try
    FreeAndNil(AnObject);
    Result :=  True;
  except
    Result := False;
  end;
end;

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
    IsOK1 : Boolean;
    IsOK2 : Boolean;
begin
  cds1      := Nil;
  cds2      := Nil; 
 try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);    
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    IsOk2 := SafeFreeAndNil(cds2);    // an error in freeing cds2 won't stop execution
    IsOK1 := SafeFreeAndNil(Cds1);
    if not(IsOk1 and IsOk2) then
      raise EWhatever....
  end;
end;

Answer 5

There is a good video on exceptions in constructors & destructors

It shows some nice examples such as:

var cds1  : TClientDataSet;
    cds2  : TClientDataSet;
begin
  cds1      := Nil;
  cds2      := Nil; 
 try
    cds1      := TClientDataSet.Create(nil);
    cds2      := TClientDataSet.Create(nil);    
    ///////////////////////////////////////////////////////////////////////
    ///      DO WHAT NEEDS TO BE DONE
    ///////////////////////////////////////////////////////////////////////
  finally
    freeandnil(cds2);    //// what has if there in an error in the destructor of cds2
    freeandnil(Cds1);
  end;
end;

What has if there in an error in the destructor of cds2

Cds1 will not be Destroyed

EDIT

Another good resource is :

Jim McKeeth excellent video on Delayed Exception Handling in code range III were he talks about problems in handling exceptions in the finally block.

Answer 6

@mghie: Delphi has got stack allocated objects:

type
  TMyObject = object
  private
    FSomeField: PInteger;
  public
    constructor Init;
    destructor Done; override;
  end;

constructor TMyObject.Init;
begin
  inherited Init;
  New(FSomeField);
end;

destructor TMyObject.Done;
begin
  Dispose(FSomeField);
  inherited Done;
end;

var
  MyObject: TMyObject;

begin
  MyObject.Init;
  /// ...
end;

Unfortunately, as the above example shows: Stack allocated objects do not prevent memory leaks.

So this would still require a call to the destructor like this:

var
  MyObject: TMyObject;

begin
  MyObject.Init;
  try
    /// ...
  finally
    MyObject.Done;
  end;
end;

OK, I admit it, this is very nearly off topic, but I thought it might be interesting in this context since stack allocated objects were mentioned as a solution (which they are not if there is no automatic destructor call).