How do you pass objects between functions?

Question

Suppose I have a function:

function someFunction: TStringList;  
begin  
  result:=TStringList.Create;  
  if someConditionIsTrue then  
    result:=doSomething;  
  //other code  
end;

And the function doSomething:

function doSomething: TStringList;
begin
  result:=TStringList.Create;
  result.Add(something);
end;

If I run this code everything works as hoped, but I'm still wondering if this is the "proper" way to pass around an object like a stringlist?

The stringlists are never freed, and I wonder if passing around objects this way could get complicated or confusing when it came to debugging or someone else trying to understand the code.

You're already causing a memory leak in the first code sample alone, without even using it anywhere else. First you create an instance, then you completely disregard that instance and create another one in its place. — Jerry Dodge, Jul 05 '16 at 17:21
Your code is creating 2 TStringlist instances, which will cause a memory leak. And the one returned by someFunction will be different depending on the value of `someConditionIsTrue`. Iow, you don't need the .Create in doSomething. — MartynA, Jul 05 '16 at 17:22
There is no "proper" way. You need to develop your convention about **which party frees memory** and strictly follow it. Also, you need to attend to compiler hints, it points you to the problem with the first snippet. — Free Consulting, Jul 05 '16 at 17:36
Also, has been discussed before many times, this one for example https://stackoverflow.com/questions/1894217 — Free Consulting, Jul 05 '16 at 17:43
@FreeConsulting -- I appreciate your response. Just for the record, though, my compiler (Lazarus/Free Pascal) is not supplying any hints about this issue. — Al C, Jul 05 '16 at 17:46
@AlC, in the nutshell, you are allocating memory with 1st assignment to `Result` and overwriting pointer to it with 2nd assignment, while you have to preserve that pointer to free that memory. Basically just the same problem you can encounter when you call the functions of this design. — Free Consulting, Jul 05 '16 at 17:58
AI C - you can work around that issue by using ARC-enabled interface from Jedi CodeLib (which is supported by FPC) - `iJclStringList` instead of `TStrings/TStringList` pointers. This would auto-free unused stringlists. Still it would be not very efficient to allocate a stringlist object just to immediately delete it and create a new one. — Arioch 'The, Jul 05 '16 at 20:39
@AIC While you probably can get around the issue as you describe, it is still not a good habit to get into. Always best to free it yourself, it's a good practice to observe. Never rely on a third party to do what you should be doing. :) — Admiral Noisey Bottom, Jul 06 '16 at 02:40

Jerry Dodge · Accepted Answer · 2016-07-05T23:08:45.683

The "proper" approach is for you to establish your own rules for how things get destroyed. It's fine to create objects in a function result, but only if you are following your own strict rules.

In your case, SomeFunction has a memory leak. First, you create a TStringList, and then if some condition is met, you create another TStringList in its place, completely disregarding the first one. Thus, leaking memory.

DoSomething shouldn't be a function returning a string list, if there's a possibility that you might already have one created. Instead, just make it a procedure...

procedure DoSomething(AList: TStringList);
begin
  AList.Add(Something);
end;

Once you do that, then SomeFunction should look like this:

function someFunction: TStringList;  
begin  
  Result:= TStringList.Create;  
  if someConditionIsTrue then  
    DoSomething(Result);  
  //other code  
end;

"The stringlists are never freed"

I hope this isn't by design. Everything you create must be free'd at some point, especially if you have functions that create their result. The only exception is if you create something that lives for the entire duration of the application, and even then it's extreme common-ground to free them anyway.

On that note, the only time I ever create an object in a function result is when I'm encapsulating multiple lines of code which would otherwise be duplicated many times. For example, creating a query.

Instead of repeating this code...

Q:= TADOQuery.Create(nil);
Q.Connection:= MyDatabaseConnection;
Q.SetSomeOtherProperties;

...I put it in a function...

function CreateQuery: TADOQuery;
begin
  Result:= TADOQuery.Create(nil);
  Result.Connection:= MyDatabaseConnection;
  Result.SetSomeOtherProperties;
end;

Then, I can simply call this function whenever I need to repeat that code...

Q:= CreateQuery;

I remember you supplying me with a nice response to a question I asked several years ago. So, thanks for responding once again :-] — Al C, Jul 05 '16 at 18:15
CreateQuery leaks of exceptions are raised after the object is created — David Heffernan, Jul 06 '16 at 00:10
@David Indeed, just a very raw and minimal code sample though. — Jerry Dodge, Jul 06 '16 at 01:21
And the asker is going to copy it and risk leaking. It's worth getting these details right. You wouldn't ever write code like that yourself I trust. Ken's answer shows how to protect the resource. — David Heffernan, Jul 06 '16 at 08:18
FWIW, about returning objects from functions: http://stackoverflow.com/questions/7423604/why-is-using-procedures-to-create-objects-preferred-over-functions/7424122#7424122 — Rudy Velthuis, Jul 06 '16 at 21:26

score 6 · Answer 2 · answered Jul 05 '16 at 17:45

The stringlists are never freed

Which is a problem in itself. Like was mentionned in comments, that creates memory leaks. In general, I frown upon functions that create objects and give ownership through their results. When I'm in need of doing so, I usually name my function "Create*" to make it as explicit as possible that the caller is responsible for freeing the memory.

With that being said, a more elegant pattern to achieve what you need :

procedure someFunction;  
var vStrings : TStringList;
begin  
  vStrings := TStringList.Create;  
  try
    if someConditionIsTrue then  
      doSomething(vStrings);  
    //other code  
  finally
    vStrings.Free;
  end;
end;

procedure doSomething(AStrings : TStringList);
begin
  AStrings.Add(something);
end;

If you really need your "someFunction" to return a TStringList and don't want to receive one through a parameter, here how to properly manage it to avoid memory leaks.

function CreateAndInitStrings : TStringList;  
begin  
  Result := TStringList.Create;  
  try
    if someConditionIsTrue then  
      doSomething(Result);  
    //other code  
  except
    Result.Free;
    raise;
  end;
end;

Err, imo `procedure someFunction` is unfortunate; maybe `procedure NotActuallyaFunction`. — MartynA, Jul 05 '16 at 18:32
I kept the same semantic as in the question to make the correspondence as explicit as possible. — Ken Bourassa, Jul 05 '16 at 20:19

How do you pass objects between functions?

2 Answers2