Lazarus

Programming => General => Topic started by: rwebb616 on May 18, 2021, 04:56:27 am

Title: Proper way to free an object returned in a function
Post by: rwebb616 on May 18, 2021, 04:56:27 am

I have a function that returns a TStringList:

Code: Pascal  [Select][+][-]

1. function TData.GetCategories: TStringList;
2. var
3.   i : integer;
4. begin
5.   result := TStringlist.Create;
6.   result.Sorted:=true;
7.   result.Duplicates:=dupIgnore;
8.   for i:=0 to Words.Count - 1 do
9.     result.Add(Tword(words[i]).Category);
10. end;

I call it as such:

Code: Pascal  [Select][+][-]

1. listbox1.items := gamedata.getcategories

heaptrc is complaining about getcategories at the line:

Code: Pascal  [Select][+][-]

1. result.Add(Tword(words[i]).Category);

First, is it the Tstringlist assigned to Result that is the issue?  and if so, how would I go about freeing it?

Title: Re: Proper way to free an object returned in a function
Post by: Handoko on May 18, 2021, 05:23:31 am

I usually will avoid creating new object as the function result. Instead I use
procedure SomeProcess(out Data: TData);

I remember there were a similar topic some months ago, that discussion is worth to read.

Title: Re: Proper way to free an object returned in a function
Post by: Leledumbo on May 18, 2021, 05:25:18 am

Quote from: rwebb616 on May 18, 2021, 04:56:27 am

I call it as such:

Code: Pascal  [Select][+][-]

1. listbox1.items := gamedata.getcategories

...
First, is it the Tstringlist assigned to Result that is the issue?  and if so, how would I go about freeing it?

Bad idea, use ListBox1.Items.Assign instead, assign the function result to a local variable first then free it after calling .Assign:

Code: Pascal  [Select][+][-]

1. lcats := gamedata.getcategories;
2. with lcats do
3.   try
4.     listbox1.items.assign(lcats);
5.   finally
6.     lcats.Free;
7.   end;
8.  

Quote from: rwebb616 on May 18, 2021, 04:56:27 am

heaptrc is complaining about getcategories at the line:

Code: Pascal  [Select][+][-]

1. result.Add(Tword(words[i]).Category);

Need to know what TWord.Category is first. If it's also a function, including property mapped one, then it's the same thing as above.

Title: Re: Proper way to free an object returned in a function
Post by: engkin on May 18, 2021, 05:26:37 am

This assignment:

Code: Pascal  [Select][+][-]

1.     listbox1.items := gamedata.getcategories;

Calls the property setter:

Code: Pascal  [Select][+][-]

1. procedure TCustomListBox.SetItems(Value: TStrings);
2. begin
3.   if (Value <> FItems) then
4.   begin
5.     LockSelectionChange;
6.     FItems.Assign(Value);
7.     UnlockSelectionChange;
8.   end;
9. end;

As you can see, it simply copies the items from the TStringList. Simply call Free after the assignment:

Code: Pascal  [Select][+][-]

1.     categories:=gamedata.getcategories;
2.     listbox1.items := categories;
3.     categories.Free;

A better way is to pass listbox1.items to gamedata.fillcategories.

Title: Re: Proper way to free an object returned in a function
Post by: eny on May 21, 2021, 01:55:48 pm

Quote from: engkin on May 18, 2021, 05:26:37 am

A better way is to pass listbox1.items to gamedata.fillcategories.

+1

Title: Re: Proper way to free an object returned in a function
Post by: Warfley on May 21, 2021, 02:41:20 pm

Having classes as return types for functions is a bad idea, because it can make things very complicated.

For example:

Code: Pascal  [Select][+][-]

1. function foo: TSomeClass;
2. begin
3.   Result := TSomeClass.Create;
4.   Result.DoSomething;
5. end;

What happens if that DoSomething throws an exception? Well the instance reference is lost. So what you need to do is to always have a try-except block in such a function:

Code: Pascal  [Select][+][-]

1. function foo: TSomeClass;
2. begin
3.   Result := TSomeClass.Create;
4.   try
5.     Result.DoSomething;
6.   except on e: Exception do
7.     FreeAndNil(Result);
8.     raise e;
9.   end;
10. end;

If you simply pass that instance as a parameter:

Code: Pascal  [Select][+][-]

1. procedure foo(inst: TSomeClass);
2. begin
3.   inst.DoSomething;
4. end;

This is not required, as you typically have a try-finally block on the callsite anyway:

Code: Pascal  [Select][+][-]

1. someInstance := TSomeClass.Create;
2. try
3.   foo(someInstance);
4.   ...
5. finally
6.   someInstance.Free;
7. end;

So giving the instance as parameter makes it much easier because you don't need a try-except block, which is required if the instance is created by the function, and more importantly, it is also cleaner because creation and destruction happens in the same block/function