Recent

Author Topic: good style: create / function / free  (Read 1323 times)

Nicole

  • Full Member
  • ***
  • Posts: 225
good style: create / function / free
« on: August 02, 2022, 11:42:45 am »
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

  • Hero Member
  • *****
  • Posts: 1050
Re: good style: create / function / free
« Reply #1 on: August 02, 2022, 12:08:51 pm »
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

  • Sr. Member
  • ****
  • Posts: 382
Re: good style: create / function / free
« Reply #2 on: August 02, 2022, 12:24:06 pm »
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

  • Hero Member
  • *****
  • Posts: 1050
Re: good style: create / function / free
« Reply #3 on: August 02, 2022, 12:27:08 pm »
Yes, on a form this works:

Code: Pascal  [Select][+][-]
  1. ThisBox := TListBox.Create(Self);

But only if it is a component.

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 10176
  • FPC developer.
Re: good style: create / function / free
« Reply #4 on: August 02, 2022, 01:15:15 pm »
Better use freeandnil, since otherwise a result:=o; might return an already freed value.

MarkMLl

  • Hero Member
  • *****
  • Posts: 4773
Re: good style: create / function / free
« Reply #5 on: August 02, 2022, 02:50:13 pm »
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".

Quote
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
MT+86 & Turbo Pascal v1 on CCP/M-86, multitasking with LAN & graphics in 128Kb.
Pet hate: people who boast about the size and sophistication of their computer.
GitHub repositories: https://github.com/MarkMLl?tab=repositories

ASerge

  • Hero Member
  • *****
  • Posts: 1942
Re: good style: create / function / free
« Reply #6 on: August 05, 2022, 08:25:40 pm »
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

  • Hero Member
  • *****
  • Posts: 527
Re: good style: create / function / free
« Reply #7 on: August 05, 2022, 10:54:49 pm »
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;

derek.john.evans

  • Jr. Member
  • **
  • Posts: 66
Re: good style: create / function / free
« Reply #8 on: August 06, 2022, 02:32:46 am »
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.
« Last Edit: August 06, 2022, 02:36:01 am by derek.john.evans »

derek.john.evans

  • Jr. Member
  • **
  • Posts: 66
Re: good style: create / function / free
« Reply #9 on: August 06, 2022, 02:54:10 am »
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.
« Last Edit: August 06, 2022, 03:04:13 am by derek.john.evans »

HeavyUser

  • Sr. Member
  • ****
  • Posts: 382
Re: good style: create / function / free
« Reply #10 on: August 06, 2022, 03:06:13 am »
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.

derek.john.evans

  • Jr. Member
  • **
  • Posts: 66
Re: good style: create / function / free
« Reply #11 on: August 06, 2022, 03:31:42 am »
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

« Last Edit: August 06, 2022, 03:41:57 am by derek.john.evans »

HeavyUser

  • Sr. Member
  • ****
  • Posts: 382
Re: good style: create / function / free
« Reply #12 on: August 06, 2022, 04:53:55 am »
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

  • Hero Member
  • *****
  • Posts: 11774
Re: good style: create / function / free
« Reply #13 on: August 06, 2022, 08:32:21 am »
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.
« Last Edit: August 06, 2022, 08:33:55 am by Thaddy »
Black themes should be banned.

440bx

  • Hero Member
  • *****
  • Posts: 2933
Re: good style: create / function / free
« Reply #14 on: August 06, 2022, 09:05:30 am »
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.
« Last Edit: August 06, 2022, 09:07:02 am by 440bx »
FPC v3.0.4 and Lazarus 1.8.2 on Windows 7 64bit.

 

TinyPortal © 2005-2018