good style: create / function / free

[Nicole]

Here is a general topic, I wondered, how to solve it in good style.
Untested code:

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

1. function Any(needle:string): TListBox;
2.  
3. begin
4.   result:=TListBox.create;  
5.   *any conditions *
6. end;

In result, there is a ListBox created.
Where is it destroyed?

Does it to have destroyed by the sender?
If yes, this is a source of mess, because I have code with is torn in pieces and tends to create - mistakes.

Thank you for all hints, how you deal with it.

[SymbolicFrank]

The best way is to create the free immediately after the create. If you have a function in which a lot of stuff happens, use this:

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

1. o := TObject.Create;
2. try
3.   // all the other code goes here
4. finally
5.   o.Free;
6. end;

If you return it as a function result, you have to free it in the calling function. Some people prefer to create it in the calling function and pass it to the function as a "var" parameter instead, but I think that's worse.

If you are not sure, ask Thaddy about smart pointers (they use ref counting).

[HeavyUser]

The best way is to create the free immediately after the create. If you have a function in which a lot of stuff happens, use this:

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

1. o := TObject.Create;
2. try
3.   // all the other code goes here
4. finally
5.   o.Free;
6. end;

If you return it as a function result, you have to free it in the calling function. Some people prefer to create it in the calling function and pass it to the function as a "var" parameter instead, but I think that's worse.

If you are not sure, ask Thaddy about smart pointers (they use ref counting).

Its a Tlistbox its shown on a form, you can't use the above pattern or smart pointers on visual controls, their life expectancy is different, that's why there is the owner mechanism in place.

The only logical recommendation is to pass the listbox as parameter in the function (var is not required).

[SymbolicFrank]

Yes, on a form this works:

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

1. ThisBox := TListBox.Create(Self);

But only if it is a component.

[marcov]

Better use freeandnil, since otherwise a result:=o; might return an already freed value.

[MarkMLl]

I'm flipping the order of your question here.

Does it to have destroyed by the sender?

Don't use "sender" in this context. "Sender" is appropriate for GUI events which are queued and handled asynchronously, in the current context use "caller".

In result, there is a ListBox created.
Where is it destroyed?

It has to be freed (preferably, FreeAndNilled) by whatever called it. As such, I think it is very good practice to emphasise that fact in the function's comment, and then to use try-finally-end.

Hence

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

1. (* This function returns a newly-created TListBox which must be freed
2.   by the caller.
3. *)
4. function Any(needle:string): TListBox;
5.      
6. begin
7.   result:=TListBox.create;  
8.   *any conditions *
9. end;
10.  
11. ...
12.  
13. tempListBox := Any('somestuff');
14. try
15. // Do stuff here
16. finally
17.   FreeAndNil(tempListBox)
18. end;
19.  

Finally, irrespective of any examples you see elsewhere, I suggest never doing something like

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

1. with Any('somestuff') do begin
2. // Do stuff here
3.   Free
4. end;
5.  

If you don't believe me, go ahead and try...

MarkMLl

[ASerge]

Some people prefer to create it in the calling function and pass it to the function as a "var" parameter instead, but I think that's worse.

Because in the case of a function, it is necessary to provide a cleaning mechanism in case of an error inside the function. It's like calling the destructor if an exception occurs in the constructor.

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

1. function Any(needle:string): TListBox;
2. begin
3.   Result := TListBox.Create;
4.   try
5.     // any conditions
6.   except
7.     Result.Free;
8.     raise;
9.   end;
10. end;

[kupferstecher]

If yes, this is a source of mess, because I have code with is torn in pieces and tends to create - mistakes.

In such cases and if appropriate I add "Create" to the function name so that its clear an instance is created:

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

1.     function CreateAny(needle:string): TListBox;
2.     begin
3.       Result := TListBox.Create;
4.       //...
5.     end;

[dje]

The only logical recommendation is to pass the listbox as parameter in the function (var is not required).

Dito!!! I'd say the question itself is incorrect (problematic), since the answer is not "good style".

I keep _all_ allocations at the same program depth level, therefore I _never_ return allocated objects from functions.
I know a number of freepascal functions do, but I personally class it as not "good style". An example is FindAllDirectories() in FileUtils:

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

1. function FindAllDirectories(const SearchPath : string;
2.   SearchSubDirs: Boolean = True): TStringList;
3. begin
4.   Result := TStringList.Create;
5.   FindAllDirectories(Result, SearchPath, SearchSubDirs);
6. end;  

Not good. But, its there and, IMHO should be removed.

Its pretty obvious, But, you have these options (there are a few others): (NOTE: I made up code for style demo purposes. The feature implemented here is NOT part of the question)

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

1. function ListBoxSearch(AListBox: TListBox; const ANeedle: string): integer;
2. var
3.   I: integer;
4. begin
5.   for I := 0 to AListBox.Items.Count - 1 do begin
6.     if SameText(AListBox.Items[I], ANeedle) then begin
7.       Exit(I);
8.     end;
9.   end;
10.   Result := -1;
11. end;
12.  
13. function StringsSearch(AStrings: TStrings; const ANeedle: string): integer;
14. var
15.   I: integer;
16. begin
17.   for I := 0 to AStrings.Count - 1 do begin
18.     if SameText(AStrings[I], ANeedle) then begin
19.       Exit(I);
20.     end;
21.   end;
22.   Result := -1;
23. end;
24.  
25. type
26.   TStringsHelper = class helper for TStrings
27.     function _Search(const ANeedle: string): integer;
28.   end;
29.  
30. function TStringsHelper._Search(const ANeedle: string): integer;
31. var
32.   I: integer;
33. begin
34.   for I := 0 to Count - 1 do begin
35.     if SameText(Self[I], ANeedle) then begin
36.       Exit(I);
37.     end;
38.   end;
39.   Result := -1;
40. end;    

Which can then be used as:

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

1.   ListBoxSearch(ListBox1, 'something');
2.   StringsSearch(ListBox1.Items, 'something');
3.   ListBox1.Items._Search('something');  

As far as "good style". I always namespace my functions by the class type name. Overloading works, but can fail or appear confusing. Class helpers are nice, but I prepend helper functions with an underscore so I know its one of my helpers, and not a class method.

Anyway, those are my thoughts. There are a number of reasons why I adhere to these rules like its a religion.

I guess for everyone, you have to try all kinds of coding methodologies to find what works for you. Lord knows, a lot of my "style" has been questioned by people.

[dje]

In result, there is a ListBox created.
Where is it destroyed?

Short answer = Within eye sight.

Using the helper implementation from above, you could use:

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

1.   with TListBox.Create(Self) do begin
2.     try
3.       Items.LoadFromFile('some file.txt');
4.       MyResult := Items._Search('something');
5.     finally
6.       Free;
7.     end;
8.   end;

I can "see" the allocation, free and exception handing in a single view. Hence why I say, create/destroy objects at the same code level. So, if an object is stored in a TForm variable, I only create/destroy in OnCreate, OnDestroy.

I've seen a lot of code which doesn't adhere to this rule, and its a nightmare to read and fix.

It often requires a rewrite, and at the very least, a lot of reading/flow tracing to find the memory leaks, dangling pointers and unhandled exceptions.

[HeavyUser]

In result, there is a ListBox created.
Where is it destroyed?

Short answer = Within eye sight.

Using the helper implementation from above, you could use:

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

1.   with ListBox1.Create(Self) do begin
2.     try
3.       Items.LoadFromFile('some file.txt');
4.       MyResult := Items._Search('something');
5.     finally
6.       Free;
7.     end;
8.   end;

I can "see" the allocation, free and exception handing in a single view. Hence why I say, create/destroy object at the same code level. So, if an object is a TForm variable, I only create/destroy in OnCreate, OnDestroy.

I've seen a lot of code which doesn't adhere to this rule, and its a nightmare to read and fix. Often requires a rewrite, and at the very least, a lot of reading/flow tracing to find the memory leaks, dangling pointers and unhanded exceptions.

Is "_Search" a custom method what it does exactly and how its declared? I would use a Tstringlist for this kind of work, there is no reason to create a visual control if its not shown on screen. In any case pass the listbox in to any function as a parameter to set it up if you want to pair creation and destruction in the same piece of code.

[dje]

Is "_Search" a custom method what it does exactly and how its declared? I would use a Tstringlist for this kind of work, there is no reason to create a visual control if its not shown on screen. In any case pass the listbox in to any function as a parameter to set it up if you want to pair creation and destruction in the same piece of code.

_search is an example TStrings helper (code in previous message). TStringList is derived from TStrings, which means you can use such an implementation on visual controls which also implement TStrings access to its items. ie: TListBox, TMenu, TListView SubItems, etc. Obviously it can be used on TStringList's too.

I agree, there doesn't seem to be a valid reason why TListBox would need to be created dynamically, let alone created for a search style function (ie: needle), but...

I tried to explain my views in the context of the question. I find this kinda question concerning, because it can lead to hundreds of lines of code written, without suitable "correct" guidance.

eg: A recent post was about an image editing example on github. I thought it was a great effort, but its over a thousand lines of code, written incorrectly. Far from "job quality" grade, and I get concerned about future coders learning bad style. It doesn't help anyone.

Note: This is what im concerned about. A very good example of how a project can go off the rails, regardless of very good intentions.
https://forum.lazarus.freepascal.org/index.php/topic,60106.msg448844.html#msg448844

[HeavyUser]

Is "_Search" a custom method what it does exactly and how its declared? I would use a Tstringlist for this kind of work, there is no reason to create a visual control if its not shown on screen. In any case pass the listbox in to any function as a parameter to set it up if you want to pair creation and destruction in the same piece of code.

_search is an example TStrings helper (code in previous message). TStringList is derived from TStrings, which means you can use such an implementation on visual controls which also implement TStrings access to its items. ie: TListBox, TMenu, TListView SubItems, etc. Obviously it can be used on TStringList's too.

I agree, there doesn't seem to be a valid reason why TListBox would need to be created dynamically, let alone created for a search style function (ie: needle), but...

I tried to explain my views in the context of the question. I find this kinda question concerning, because it can lead to hundreds of lines of code written, without suitable "correct" guidance.

eg: A recent post was about an image editing example on github. I thought it was a great effort, but its over a thousand lines of code, written incorrectly. Far from "job quality" grade, and I get concerned about future coders learning bad style. It doesn't help anyone.

Note: This is what im concerned about. A very good example of how a project can go off the rails, regardless of very good intentions.
https://forum.lazarus.freepascal.org/index.php/topic,60106.msg448844.html#msg448844

Oh I see. I missed your previous post, sorry.
The code shown does create a bad habit vibe, but unless the OP clarifies on how and where the listbox is used ee attached to a form or not, there is no correct answer. Even the discussion brings out patterns that are in the good habit group but might not be appropriate for the way the control is used.

Sorry I don't know enough to decide on what might be a good code for this case. In general use, you are correct.

[Thaddy]

Better use freeandnil, since otherwise a result:=o; might return an already freed value.

And thus supporting/encouraging programmer error? Use after free? That's the reason I hate freeandnil...
It certainly does not belong to good programming style. A good programming style prevents use after free.

[440bx]

And thus supporting/encouraging programmer error? Use after free? That's the reason I hate freeandnil...
It certainly does not belong to good programming style. A good programming style prevents use after free.

Resetting a pointer to nil when the memory block it pointed to is freed is hardly encouraging programming errors.  Actually, it _is_ a programming error _not_ to reset the pointer to nil after freeing the block it pointed to (since after the block is freed the pointer value is no longer valid.)

Good programming style and, just plain good programming : don't leave pointers with stale/invalid values.(pointing to somewhere in memory, i.e, not nil.)

Good programming style doesn't prevent use after free, people (not you, of course) make mistakes and good programming style causes mistakes to be revealed very shortly after they are made.  If a nil pointer is dereferenced an access violation will occur immediately.  If a pointer that has a stale value is dereferenced, the problem that dereference caused may only become obvious hundreds or even thousands of lines of code later, producing very hard to find bugs.