TStringList property in custom class constructor doesn't persist

[FiftyTifty 0]

I'm working on programs that make modding Conan Exiles much easier, as the editor is very lacking. The game heavily relies upon databases, and the editor is like a very, very basic spreadsheet. Luckily, it supports importing and exporting JSON data, so I've something to work with. I will say that I'm completely self taught at programming, and had a mentor back when I first started learning how to use Delphi.

 

Now, here is the problem. I've a custom class CEItemStatModification, which has a TStringList property CEItemStatModification.Modifications. The constructor runs and creates the TStringList, but for some reason, I have to call .create; on it again in my actual code, otherwise it's invalid and doesn't return anything. Maybe it's getting destroyed for some reason?

 

Here's the repository, with libraries being up one folder: https://github.com/FiftyTifty/Conan-Exiles/tree/master/Conan Exiles - ItemStatModification Editor

Here's the JSON data: https://pastebin.com/Zei7W190

 

Here's the custom class code: https://github.com/FiftyTifty/Conan-Exiles/blob/master/Conan Exiles - API/fytyConanExilesTypes.pas

 

Specifically, here it is along with the constructor:

 

  CEItemStatModification = class
    public
    [JsonName('RowName')]
    RowName : string;
    [JsonName('Modifications')]
    Modifications : TStringList;
    published
    constructor Create;
    destructor Destroy;
  end;

  ptrCEItemStatModification = ^CEItemStatModification;

implementation

  constructor CEItemStatModification.Create;
  begin
    //ShowMessage('Created TStringList!');
    Modifications := TStringList.Create;
  end;

  destructor CEItemStatModification.Destroy;
  begin
    ShowMessage('Destroyed object!');
    Modifications.Free;
  end;

end.

That ShowMessage() line prints properly with each object created in my main code. But it doesn't persist. If I don't call TStringList.Create again in my code, no string data will be returned:

 

procedure TformWindow.buttonLoadClick(Sender: TObject);
var
  ...
begin

	...

	for djsonEntry in djsonDataTable do begin

		//ShowMessage('Iteration number: '+ iCounter.ToString);

		ceismToAdd := CEItemStatModification.Create;
		ceismToAdd.Modifications := TStringList.Create;

		//ShowMessage('Created CEItemStatModification!');

		for djsonISMod in djsonEntry['Modifications'] do begin
		  //ShowMessage(djsonISMod.AsString);
		  ceismToAdd.Modifications.Add(djsonISMod.AsString);
		end;
	...

But that brings a new problem. Every created object will have the exact same TStringList data as the first created object. And I can't figure out why. Any ideas?

[Remy Lebeau 1393]

1 hour ago, FiftyTifty said:

ceismToAdd := CEItemStatModification.Create; ceismToAdd.Modifications := TStringList.Create;

This is absolutely a memory leak.  You need to get rid of the 2nd TStringList.Create.  But you didn't really explain HOW your code fails when you don't make that call.

 

I see nothing in the code you have provided that can be freeing the CEItemStatModification.Modifications object prematurely.  I do not see you using a JSON streaming library that saves your objects to a stream, or reads a stream into your objects (so why are you decorating your class fields with JSON attributes?), so I don't see what persistence has to do with anything.

 

You are just going to have to debug your code to find out exactly what is happening.  Put a breakpoint inside your constructor, make sure it is being called correctly.  Put a breakpoint after the constructor, make sure the newly constructed object is still proper after being constructed.  Put breakpoints before and after every step of the code that touches your objects, make sure the objects are still proper after every step.  As soon as you find a deviation, STOP, repeat, and trace into the step that didn't act as expected.

[FiftyTifty 0]

4 minutes ago, Remy Lebeau said:

This is absolutely a memory leak.  You need to get rid of the 2nd TStringList.Create.  But you didn't really explain HOW your code fails when you don't make that call.

 

I see nothing in the code you have provided that can be freeing the CEItemStatModification.Modifications object prematurely.  I do not see you using a JSON streaming library that saves your objects to a stream, or reads a stream into your objects (so why are you decorating your class fields with JSON attributes?), so I don't see what persistence has to do with anything.

 

You are just going to have to debug your code to find out exactly what is happening.  Put a breakpoint inside your constructor, make sure it is being called correctly.  Put a breakpoint after the constructor, make sure the newly constructed object is still proper after being constructed.  Put breakpoints before and after every step of the code that touches your objects, make sure the objects are still proper after every step.  As soon as you find a deviation, STOP, repeat, and trace into the step that didn't act as expected.

My apologies, I'll clarify. If I don't call TStringList.Create again in my code, no string data will be returned. As in, the TStringList property .Modifications of my class will return completely blank strings. So for some reason, the TStringList that is created in my class' constructor isn't persisting for some reason.

 

So here's the JSON situation. Conan Exiles' devkit allows exporting the databases as JSON text. So I made a memo that you paste the JSON into, and using the Delphi-JSON library, I read the data inside each JSON object into another object of my class. Here's the code without the commented out ShowMessage() lines:

 

procedure TformWindow.buttonLoadClick(Sender: TObject);
var
  iCounter, iNumModifications, iCounterModifications,
  iAddedIndex: integer;
  treeEntry: TTreeNode;
  ceismToAdd: CEItemStatModification;
  djsonEntry, djsonISMod: TDJSON;
begin

  djsonDataTable := TdJSON.Parse(formWindow.formSource.Lines.Text);

  iCounter := 0;

  if listCEItemStatModification.Count > 0 then
    listCEItemStatModification.Clear;


  for djsonEntry in djsonDataTable do begin


    ceismToAdd := CEItemStatModification.Create;
    ceismToAdd.Modifications := TStringList.Create;

    for djsonISMod in djsonEntry['Modifications'] do begin
      ceismToAdd.Modifications.Add(djsonISMod.AsString);
    end;

    ceismToAdd.RowName := djsonEntry['RowName'].AsString;

    listCEItemStatModification.Add(ceismToAdd);

    treeEntry := formTree.Items.AddChildObject(nil, djsonEntry['RowName'].AsString, listCEItemStatModification.Items[iCounter]);
    if tstrlistItemIDs.IndexOf(treeEntry.Text) >= 0 then
      treeEntry.Text := treeEntry.Text + ' - ' + tstrlistItemNames[(tstrlistItemIDs.IndexOf(treeEntry.Text))]
    else

    Inc(iCounter);

  end;

Then once the data has been modified, the user will just need to press the save button, and my objects will be output into the bottom memo as JSON data. Ctrl+A -> Ctrl+C -> Open Devkit -> Ctrl+V. Easy.

 

Don't know a thing about breakpoints, but I essentially did the exact same thing but with ShowMessage() outputting the data after every line. The JSON data is read from the memo fine. The JSON data is read into the class fine. What fails, is when the objects are added to the TList listCEItemStatModification, they all have the exact same data in their .Modifications property as the very first object added to the TList.

[Remy Lebeau 1393]

20 minutes ago, FiftyTifty said:

If I don't call TStringList.Create again in my code, no string data will be returned.

Returned WHERE exactly?  WHERE are you expecting the strings to be when you see them missing?

20 minutes ago, FiftyTifty said:

As in, the TStringList property .Modifications of my class will return completely blank strings.

Sorry, but given the code you have provided, that is simply not possible.

20 minutes ago, FiftyTifty said:

So for some reason, the TStringList that is created in my class' constructor isn't persisting for some reason.

Persisting WHERE exactly?

20 minutes ago, FiftyTifty said:

So I made a memo that you paste the JSON into, and using the Delphi-JSON library, I read the data inside each JSON object into another object of my class.

But in the code you have provided, you are doing all of that manually, there is no automated persistence involved.  What am I missing here?

20 minutes ago, FiftyTifty said:

Then once the data has been modified, the user will just need to press the save button, and my objects will be output into the bottom memo as JSON data.

Where is THAT code?   The Save button in the Editor code you provided has no OnClick handler or TAction assigned to it.  I see NO CODE saving the object data ANYWHERE.

20 minutes ago, FiftyTifty said:

Don't know a thing about breakpoints

Seriously?  How do you NOT know how to work with breakpoints?  Debugging is an essential skill for a programmer, and breakpoints are a big part of that.

20 minutes ago, FiftyTifty said:

I essentially did the exact same thing but with ShowMessage() outputting the data after every line.

The ShowMessage() calls in the code you have provided are not adequate enough to see what is really happening.  They are only being used to show the flow of the code, but not the state of the code.

 

Oh, BTW, I just now saw a bug in one of your ShowMessage() calls:

//ShowMessage('Added ItemStatModification to master list! ' + listCEItemStatModification[0].RowName);

Needs to be changed to this instead:

//ShowMessage('Added ItemStatModification to master list! ' + ceismToAdd.RowName);

Or at least to this (though the above is preferred):

//ShowMessage('Added ItemStatModification to master list! ' +  listCEItemStatModification.Items[iCounter].RowName);

Do you see why the original statement was faulty?  It is displaying the RowName of the 1st object in the list on every loop iteration, not the RowName of the current object that was just added to the list on each iteration.

20 minutes ago, FiftyTifty said:

The JSON data is read from the memo fine. The JSON data is read into the class fine.

How do you know that, if you don't know how to use the debugger to actually step through the code in real time and look at the data in memory?

20 minutes ago, FiftyTifty said:

What fails, is when the objects are added to the TList listCEItemStatModification, they all have the exact same data in their .Modifications property as the very first object added to the TList.

Sorry, but given the code you have shown, there is absolutely no possible way that can be happening.

 

That said, I would also like to point out that I see other bugs in your code, too.  In particular, if the Load button is clicked multiple times, I see several memory leaks occurring, but more importantly you are not clearing the TreeView of old nodes when you clear the TList (leaking the objects in it!) that backs the TreeView, and before you load the new data into the TreeView.  You just keep adding new nodes to the TreeView but never remove them.

 

I think you are misdiagnosing your problem, if your observations are based on the logic bugs I mentioned above, and not due to any failure of the actual reading/saving code.

[FiftyTifty 0]

I actually found the problem when I was going to uncomment my ShowMessage() lines. See this wee block?

 

    ShowMessage(IntToStr(listCEItemStatModification.Add(ceismToAdd)));
    ShowMessage('Added ItemStatModification to master list! ' + ceismToAdd.RowName);
    ShowMessage(IntToStr(iCounter));

    treeEntry := formTree.Items.AddChildObject(nil, djsonEntry['RowName'].AsString, listCEItemStatModification.Items[iCounter]);
    //ShowMessage('Added tree node!');
    if tstrlistItemIDs.IndexOf(treeEntry.Text) >= 0 then
      treeEntry.Text := treeEntry.Text + ' - ' + tstrlistItemNames[(tstrlistItemIDs.IndexOf(treeEntry.Text))];
    else
    //ShowMessage(treeEntry.text);

    Inc(iCounter);

Forgot to comment the else statement. In my defence, I've been doing my coding at 2am.

 

[David Schwartz 426]

Here are a few random comments about other "code smells"....

 

You seem to be treating classes as records.

 

On 1/31/2020 at 4:41 PM, FiftyTifty said:

ptrCEItemStatModification = ^CEItemStatModification;

 

What's this for? Anything defined as a 'class' is already passed by reference. You don't need pointers for anything related to classes.

 

On 1/31/2020 at 4:41 PM, FiftyTifty said:

CEItemStatModification = class
  public
    [JsonName('RowName')]
    RowName : string;
    [JsonName('Modifications')]
    Modifications : TStringList;

  published
    constructor Create;
    destructor Destroy;
end;

 

This ^^^^^^^^ is a very unconventional coding style.

 

First, you're not inheriting this from any base class that has the ability to deal with the attributes ([JsonName(...)], etc), and this one doesn't either. So what are they doing there? As it is, they're little more than comments that are generating needless stuff in your executable.

 

The fields RowName and Modifications should be defined as properties and made 'private'.

 

And there's never any reason I can think of to make constructors or destructors 'published'. Usually they're 'public', and they can be 'protected' if they're in a base class.

 

This is more consistent with typical Delphi usage:

TCEItemStatModification = class
   private 
    FRowName : string; 
    FModifications : TStringList; 
  public
    constructor Create; 
    destructor Destroy; override;

    property RowName : string read FRowName write FRowName;
    property Modifications : TStringList read FModifications; // this is initialized by ctor, never set by the user
end;

 

Personally speaking, I'd be inclined to put the RowName as a parameter on the ctor, as well as the list of things that get added to Modifications. They're simply initializers. Why not let the ctor do that? (This would make the RowName property read-only as well, so no 'write' clause is needed.)

 

 

Edited February 6, 2020 by David Schwartz