CODEWITHSUNDEEP
delphiooptobjectlist
Bulan
Bulan

Reputation: 713

Delphi: Correct way to store objects fetched from TObjectList

This example is of course simplified, but basically I have a main form that triggers another form (frmSettings) with 

function Execute(var aSettings: TSettings):Boolean

TSettings is my own object created in main form for keeping track of the settings.

In this newly opened form (frmSettings) I fetch a TMyObjectList that is a descendant from TObjectList. It's filled with TMyObj.

I then fill a TListBox with values from that TMyObjectList.

the code:

...

FMyObjectList : TMyObjectList;
property MyObjectList: TMyObjectList read getMyObjectList;

...

function TfrmSettings.getMyObjectList: TMyObjectList ;
begin
    If not Assigned(FMyObjectList) then FMyObjectList := TMyObjectList.Create(True)
    Result := FMyObjectList;
end;

function TfrmSettings.Execute(var aSettings: TSettings): Boolean;
begin

    //Fill myObjectList
    FetchObjs(myObjectList);

    //Show list to user
    FillList(ListBox1, myObjectList);

    //Show form        
    ShowModal;

    Result := self.ModalResult = mrOk;

    if Result then
    begin
        // Save the selected object, but how??

        // Store only pointer? Lost if list is destroyed.. no good
        //Settings.selectedObj := myObjectList.Items[ListBox1.ItemIndex];

        // Or store a new object? Have to check if exist already?
        If not Assigned(Settings.selectedObj) then Settings.selectedObj := TMyObj.Create;
        Settings.selectedObj.Assign(myObjectList.Items[ListBox1.ItemIndex];);
    end;

end;

procedure TfrmSettings.FillList(listBox: TListBox; myObjectList: TMyObjectList);
var
    i: Integer;
begin
    listBox.Clear;
    With myObjectList do
    begin
        for i := 0 to Count - 1 do
        begin
            //list names to user
            listBox.Items.Add(Items[i].Name);
        end;
    end;
end;

procedure TfrmSettings.FormDestroy(Sender: TObject);
begin
    FreeAndNil(FMyObjectList);
end;

Storing just the pointer doesn't seem as a good idea, as triggering the settings form again, recreates the list, and the original object would be lost even if user hits "cancel"

So storing a copy seems better, using assign to get all the properties correct. And first checking if I already have an object.

        If not Assigned(Settings.selectedObj) then Settings.selectedObj := TMyObj.Create;
        Settings.selectedObj.Assign(myObjectList.Items[ListBox1.ItemIndex];);

Should I move those two lines to a method instead like Settings.AssignSelectedObj(aMyObj:TMyObj)

Does this look correct or am I implementing this the wrong way? Something more/less needed?

I need some guidelines so I feel more secure that I don't open up for memory leaks and other trouble.

Other than that reviewing the code a bit, the real question is: Is this the correct way to store my SelectedObject in the settings class?

Upvotes: 3

Views: 2693

Answers (3)

Arnaud Bouchez
Arnaud Bouchez

Reputation: 43053

What about the serialization of TSettings? Put your settings in some published properties, then let the RTTI save its content:

type
  TSettings = class(TPersistent)
  public
    function SaveAsText: UTF8String;
  end;

function TSettings.SaveAsText: UTF8String;
begin
var
  Stream1, Stream2: TMemoryStream;
begin
  Stream1 := TMemoryStream.Create;
  Stream2 := TMemoryStream.Create;
  try
    Stream1.WriteComponent(MyComponent);
    ObjectBinaryToText(Stream1, Stream2);
    SetString(result,PAnsiChar(Stream2.Memory),Stream2.Size);
  finally
    Stream1.Free;
    Stream2.Free;
  end;
end;

Then your settings can be stored in a text file or text string.

It's just one solution. But storing settings as text is very handy. We use such an approach in our framework, to store settings via a code-generated user interface. A settings tree is created, from a tree of TPersistent instances.

Upvotes: 0

jpfollenius
jpfollenius

Reputation: 16620

Is this the correct way to store the selected object in the settings?

Probably not. Your settings class should not depend on the form in any way. What if you decide to create and destroy your form dynamically each time the user opens the settings? In this case your settings would hold an invalid object reference.

IMHO it is better to store the object list in the settings together with the index of the selected object. The form should just access the settings, fill the list box and modify the selected object index after the user confirmed with OK.

You are producing a memory leak in your code. You create a TObjectList as a local variable but you never free it. And if you free the local variable, the object references in the listbox will be invalid. You have two options:

  • Store the object list as a member variable of your form, create in the FromCreate event handler and destroy it in the FormDestroy event handler. You can then safely use object references in your list box.

  • Store the object list somewhere outside and pass it into the form as a parameter of the Execute method. In this scenario, you can also safely use object references.

Upvotes: 1

Chris Thornton
Chris Thornton

Reputation: 15817

I would rename myObjectList to GlobalObjectList, and move it out of the class. It can be declared in the form, but create/free in the initialization/finalization sections. During initialization, after you create the list, populate it from the ini file (or wherever you store it). Now you can access it from anywhere that has your unit in the Uses.

Upvotes: 0

Related Questions