Publuc property of type TStringList

[zoltanleo]

Hi guys.

I am trying to use a public property of type TStringList. Are memory leaks possible in this code like this?

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

1. type
2.   { TForm1 }
3.   TForm1 = class(TForm)
4. ...
5.     procedure FormClose(Sender: TObject; var CloseAction: TCloseAction);
6.     procedure FormCreate(Sender: TObject);
7.   private
8.     FMySL: TStringList;
9.     procedure SetMySL(AValue: TStringList);
10.   public
11.     property MySL: TStringList read FMySL write SetMySL;
12. ...
13.   end;
14.  
15. procedure TForm1.SetMySL(AValue: TStringList);
16. begin
17.   if FMySL=AValue then Exit;
18.   if Assigned(FMySL) then FreeAndNil(FMySL);
19.   FMySL:=AValue;
20. end;
21.  
22. procedure TForm1.FormCreate(Sender: TObject);
23. begin
24.   MySL:= TStringList.Create;
25. ...
26. end;
27.  
28. procedure TForm1.FormClose(Sender: TObject; var CloseAction: TCloseAction);
29. begin
30.   if Assigned(MySL) then MySL.Free;
31. end;
32.  
33. //using
34. procedure TForm1.Button3Click(Sender: TObject);
35. var
36.   i: PtrInt;
37. begin
38.   with MySL do
39.   begin
40.     Clear;
41.     for i:= 0 to 9 do
42.     begin
43.       Add('item_' + FormatDateTime('hh:nn:ss:zzz', Now));
44.       Sleep(50);
45.     end;
46.   end;
47.   Memo1.Lines.Assign(MySL);
48. end;
49.  

[rvk]

Don't do MySL.Free in FormClose but in FormDestroy.

The opposite of FormCreate is FormDestroy.
You are guaranteed both are executed.
(FormClose is not guaranteed)

[zoltanleo]

@rvk
Thank you for your comment.

Can the code inside procedure SetMySL be considered correct?

[lucamar]

Yes, it looks correct see but I would do things like this:

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

1. type
2.   { TForm1 }
3.   TForm1 = class(TForm)
4. ...
5.     procedure FormDestroy(Sender: TObject);
6.     procedure FormCreate(Sender: TObject);
7.   private
8.     FMySL: TStringList;
9. ...
10.   end;
11.  
12. procedure TForm1.FormCreate(Sender: TObject);
13. begin
14.   FMySL:= TStringList.Create;
15. ...
16. end;
17.  
18. procedure TForm1.FormDestroy(Sender: TObject);
19. begin
20.   if Assigned(FMySL) then FMySL.Free;
21. end;
22.  
23. //using
24. procedure TForm1.Button3Click(Sender: TObject);
25. var
26.   i: PtrInt;
27. begin
28.   with FMySL do
29.   begin
30.     Clear;
31.     for i:= 0 to 9 do
32.     begin
33.       Add('item_' + FormatDateTime('hh:nn:ss:zzz', Now));
34.       Sleep(50);
35.     end;
36.   end;
37.   Memo1.Lines.Assign(FMySL);
38. end;

I wouldn't bother with the property, just access the field by itself. Generally speaking a property, with its getter and setter, is nice to have when those involve more than simply assigning it like, say, checking some values to set it some way or other, or when it depends on other fields/properties, etc. For a simple list which is used by itself, declaring and using the field should be enough; no need to further complicate things.

[zoltanleo]

no need to further complicate things.

Hi lucamar.

Thank U for answer.
I have declared this property for a reason. I want to access it from other units in order to pass the contents of the list between forms.

[wp]

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

1. type
2.   { TForm1 }
3.   TForm1 = class(TForm)
4.   private
5.     FMySL: TStringList;
6.     procedure SetMySL(AValue: TStringList);
7.   public
8.     property MySL: TStringList read FMySL write SetMySL;
9. ...
10.   end;
11.  
12. procedure TForm1.SetMySL(AValue: TStringList);
13. begin
14.   if FMySL=AValue then Exit;
15.   if Assigned(FMySL) then FreeAndNil(FMySL);
16.   FMySL:=AValue;
17. end;
18.  

This is a risky construction. It is the calling program which created the stringlist passed to SetMySL as a parameter, and therefore normally it the responsibility of the calling program to destroy the stringlist. But since you just copy the pointer to TForm1 the stringlist will be gone and your program will not notice it; it even won't help when the calling routine sets the destroyed stringlist to nil -- this will affect the local variable in the calling program, but not the copied value FMySL. A crash is very likely...

Better: Keep ownership of the stringlist at the calling routine and copy the stringlist items to FMySL, not the pointer to the stringlist. You could even go a step further and declare AValue as the more general type TStrings; sticking to TStringList would not allow you to use the Lines of a TMemo in SetMySL (because this is not a TStringList but a TTextStrings which inherits from TStrings without going the TStringList path of inheritance).

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

1. procedure TForm1.SetMySL(AValue: TStrings);
2. begin
3.   FMySL.Assign(AValue);  // This copies all properties of AValue over to FMySL
4. end;

[lucamar]

I have declared this property for a reason. I want to access it from other units in order to pass the contents of the list between forms.

If it's only that then declare it as a read-only property. That would allow using it, including modyfing its contents, wherever needed but keep the ownership (hence construction/destruction responsability)  in the container form object.

[zoltanleo]

If it's only that then declare it as a read-only property.

I can't declare this property just RO because I have to be able to change this property from another unit. For example:

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

1. procedure TForm1.Button2Click(Sender: TObject);
2. var
3.   i: Integer;
4. begin
5.   FrmMain.MySL.Clear;//here MySL this is public property of FrmMain
6.   for i:= 0 to Pred(CheckListBox1.Items.Count) do
7.     if not CheckListBox1.Checked[i] then FrmMain.MySL.Add(CheckListBox1.Items.Strings[i]);//copying unchecked items from chebklistbox to FrmMain.MySL
8.  
9.   Memo1.Lines.Assign(FrmMain.MySL);//checking copied lines
10. end;

Better: Keep ownership of the stringlist at the calling routine and copy the stringlist items to FMySL, not the pointer to the stringlist.

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

1. procedure TForm1.SetMySL(AValue: TStrings);
2. begin
3.   FMySL.Assign(AValue);  // This copies all properties of AValue over to FMySL
4. end;

Hi Werner.
Thank U for the answer. Sorry but your code raises an error

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

1. Project project1 raised exception class 'External: SIGSEGV'.
2.  
3.  In file 'unit1.pas' at line 90:
4. FMySL.Assign(AValue);

In addition, using type TStrings instead of TStringList also causes errors on methods Clear, Add  etc.

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

1. Project project1 raised exception class 'RunError(211)' with message:
2. Call to abstract method
3.  
4.  In file 'unit1.pas' at line 65:
5. Clear;

I have attached a project archive for experiments

[Sieben]

As lucamar wrote, a readonly property would not prevent you from modifying it's contents like you do in line 7 of your first code snippet. Using Add, Clear, Assign etc is perfectly possible with a readonly property. Readonly just means you are not allowed to change the (pointer of the) object itself, but of course you can use and modify the contents of the object.

[Sieben]

Had a look at your project. Don't use:

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

1. TStrings.Create;  

but always

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

1. TStringList.Create;  

because TStrings is in fact just an abstract class that should never be used in 'real' code. Suggesting a procedure with parameter type TStrings like wp just means that you can pass any descendant of TStrings, which makes the procedure more widely usable (or 'generic'). 

Edit: attached corrected sample project.

[wp]

Sorry but your code raises an error

As Sieben already wrote, create FMySL as a TStringList.

However, using my setter function where the items of the string passed as a parameter are only assigned to the internal list (FMySL), you cannot do "MySL := TStringList.Create" -- this code creates a stringlist and copies its items (there are none in the beginning!) to FMySL which does not yet exist --> crash. Rather than using "MySL" use the internal variable "FMySL":

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

1. procedure TForm1.FormCreate(Sender: TObject);
2. begin
3.   FMySL:= TStringList.Create;   // FMySL instead of MySL !!!
4.   FillCheckListBox(Sender);
5. end;

[Sieben]

Correct of course, will change that in my original post to avoid confusion.

[zoltanleo]

Hi guys.

Thanks for your help and advice. I will try to apply the corrected code in a complex project.

[zoltanleo]

Hi guys.

Here is a schematic example of my real application. In my opinion, I was able to achieve the desired result using only TStringList instead of TStrings.

Can someone tell me if I wrote the setter correctly? Can TStrings be used instead of TStringList?

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

1. { TfrmMain }
2.  
3. //create and call detail form
4. procedure TfrmMain.Button1Click(Sender: TObject);
5. var
6.   tmpForm: TfrmDetail;
7. begin
8.   FMyStr:= TStringList.Create;
9.   try
10.     FMyStr.Assign(ComboBox1.Items);
11.     tmpForm:= TfrmDetail.Create(Self);
12.     try
13.       tmpForm.ShowModal;
14.       FillList(ComboBox1, MyStr);
15.       if ComboBox1.Items.Count > 0 then ComboBox1.ItemIndex:= 0;
16.     finally
17.       FreeAndNil(tmpForm);
18.     end;
19.   finally
20.     FreeAndNil(FMyStr);
21.   end;
22. end;
23.  
24. //setter
25. procedure TfrmMain.SetMyStr(AValue: TStringList);
26. begin
27.   if FMyStr=AValue then Exit;
28.   //FMyStr:=AValue;
29.   if Assigned(FMyStr) then FMyStr.Assign(AValue);
30. end;
31.  
32. { TfrmDetail }
33.  
34. procedure TfrmDetail.Button1Click(Sender: TObject);
35. var
36.   i: PtrInt;
37. begin
38.   frmMain.MyStr.Clear;
39.   for i:= 0 to Pred(CheckListBox1.Count) do
40.     if CheckListBox1.Checked[i] then frmMain.MyStr.Add(CheckListBox1.Items.Strings[i]);
41.  
42.   Close;
43. end;

[zoltanleo]

screen