Delphi function return class object

4
votes

In addition to this question I have made some tests and researches on the docwiki. My conclusion is that this kind of code should work without memory leaks:

function testResultObject: TClassA;
begin
 Result := TClassA.Create;
 Result.DoSomething;
end;

And then somewhere I can call the above code in this manner:

var k: TClassA;
begin

 k := testResultObject;
 try
  //code code code
 finally
  k.Free;
 end;

end;

As Remy suggested in the answer it's better to avoid this way of doing things and instead use something like testResultObject(x: TClassA): boolean. In this case the return true/false can tell me if everything went fine and I am passing an object already created.

Look at this code:

function testResultObject: TClassA;
begin

 Result := TClassA.Create;

 try
  Result.DoSomething;
 except
  Result.Free;
 end;

end;

The problem with the first version above of the function is that DoSomething could raise an exception and if so I'll leak memory. Can the second implementation with try-except be a solution? For sure later I'll have to check if the result is assigned or nil.

I agree that (as already said above) the testResultObject(x: TClassA): boolean would be better. I was just wondering if the return-a-class function way could be fixed as I've written.

3
delphi
Personally I have always used the second approach (the true/false return) which is better and makes the code more readable. Your solution is good but you should add also a Result := nil in the exception block – Alberto Miola
You misinterpreted my previous answer. Passing an object as a parameter is good when you are modifying that object (in that example, adding items to a list). This is a completely different scenario. – Remy Lebeau

3 Answers

9
votes

Your code has serious problems. In case of an error, it swallows the exception, and returns an invalid object reference.

This is easy to fix. The canonical way is as follows:

function testResultObject: TClassA;
begin
  Result := TClassA.Create;    
  try
    Result.DoSomething;
  except
    Result.Free;
    raise;
  end;
end;

Either the function succeeds and returns a new object. Or it fails, cleans up after itself, and raises an exception.

In other words, this function looks and behaves just like a constructor. You consume it in the same way:

obj := testResultObject;
try
  // do things with obj
finally
  obj.Free;
end;
5
votes

Your second approach works, but has 2 serious problems.

  • By swallowing all exceptions, (as J pointed out) you'll hide the fact that something went wrong.
  • There's no indication to the caller that you've created an object that the caller is responsible for destroying. This makes using the function more error prone; and easier to cause memory leaks.

I would recommend the following improvement on your second approach:

         {Name has a clue that caller should take ownership of a new object returned}
function CreateObjectA: TClassA;
begin
  {Once object is successfully created, internal resource protection is required:
      - if no error, it is callers responsibility to destroy the returned object
      - if error, caller must assume creation *failed* so must destroy object here
  Also, by assigning Result of successful Create before *try*:
      The object (reference) is returned
          **if-and-only-if**
      This function returns 'normally' (i.e. no exception state)}
  Result := TClassA.Create;    
  try
    Result.DoSomething; {that could fail}
  except
    {Cleanup only if something goes wrong:
      caller should not be responsible for errors *within* this method}
    Result.Free;
    {Re-raise the exception to notify caller:
      exception state means caller does not "receive" Result...
      code jumps to next finally or except block}
    raise;
  end;
end;

The most important benefit of the above create function is that: as far as any caller/client code is concerned, it behaves exactly like a normal TObject.Create.
And so the correct usage pattern is exactly the same.

Note that I'm not keen on J's FreeAndNil suggestion because if calling code doesn't check if the result was assigned: it is likely to AV. And code that does check the result correctly will be a little messy:

var k: TClassA;
begin
  k := testResultObject; {assuming nil result on failed create, next/similar is *required*}
  if Assigned(k) then    {Note how this differs from normal try finally pattern}
  try
    //code using k
  finally
    k.Free;
  end;
end;

NB: It's important to note that you cannot ever have your caller simply ignore memory management; which brings me to the next section.

All the above aside, there is much less chance of making careless mistakes if your testResultObject takes an input object that you require the caller to create and manage its lifetime as needed. I'm not sure why you're resisting that approach so much? You cannot get simpler than the following without resorting to a different memory model.

var k: TClassA;
begin
  k := TClassA.Create;
  try
    testResultObject(k); {Where this is simply implemented as k.DoSomething;}
    //more code using k
  finally
    k.Free;
  end;
end;
4
votes

The only problem with this :

 function testResultObject: TClassA;
 begin
   Result := TClassA.Create;    
   try
     Result.DoSomething;
   except
     Result.Free;
   end;
 end;

Is that you have no way of knowing whether the function was successful. Freeing an object does not alter the reference; the variable will still point to the (now) invalid memory location where the object used to exist. You must explicitly set the reference to nil if you want the consumer to be able to test if the reference is valid. If you want to use this pattern (having the consumer test for nil) then you would need to do :

try
  Result.DoSomething;
except
  FreeAndNil(Result);
end;

This way the caller can test the result for nil (using Assigned or otherwise) as you intended. This still isn't a very clean approach, however, since you're still swallowing exceptions. Another solution might be to simply introduce a new constructor or alter the existing one. For example

TFoo = class
  public
    constructor Create(ADoSomething : boolean = false);
    procedure DoSomething;
end;

constructor TClassA.Create(ADoSomething: Boolean = False);
begin
  inherited Create;
  if ADoSomething then DoSomething;
end;

procedure TClassA.DoSomething;
begin
  //
end;

This way you can get rid of all of the exception handling and just call this as :

function testResultObject: TClassA;
begin
  Result := TClassA.Create(true);     
end;

Since you've now pushed the DoSomething execution into the constructor any exceptions will naturally automatically call the destructor and your memory management problems go away. The other answers also have good solutions.