Should I create subclass for User interface interaction?

[Mike Torrettinni 198]

So, I have my first attempt at designing a class and I was wondering if I need to subclass to separate User interface methods and CLI (cmd) methods (when project is run without main form).

So, the class is a file parser (custom file structure, similar to XML) and I need it to ParseFile and then either to show parsed data on Form control (when run with UI) or export to file (when run in CLI mode).

 

Here is the very early version of the class design:

 

unit uParseABCFile;

interface

uses System.Classes, Vcl.StdCtrls, Vcl.Dialogs;

type


  TABCFileParser = class
    private

      type
        TABCStructure = record
          NodeName: string;
          ListOfArguments: TStringList;
        end;

      var
        FABCFileName: string;
        FABCStructure: TArray<TABCStructure>;
        // User interface Controls
        FMemo: TMemo;

      procedure MemoDblClick(Sender: TObject);

    public
      constructor Create;
      property ABCFileName: string read FABCFileName write FABCFileName;
      procedure ParseFile;

      // Methods for User Interface
      procedure ABCStructureToMemo(aMemo: TMemo);

      // Methods for non-UI mode
      procedure ABCStructureToFile(const aFileName: string);
  end;

  // Subclasses ??
  // TABCFileParser_UserInterface = class(TABCFileParser);
  // TABCFileParser_CLI = class(TABCFileParser);


implementation


constructor TABCFileParser.Create;
begin
  // add something, if needed in the future
end;

procedure TABCFileParser.ParseFile;
begin
  // Parse ABC File - FABCFileName

  // demo data
  SetLength(FABCStructure, 1);
  FABCStructure[0].NodeName := 'ABC Node 1';
  FABCStructure[0].ListOfArguments := TStringList.Create;
  FABCStructure[0].ListOfArguments.AddStrings(['Test','ID', 'Description', 'ABCVersion', 'ABCType']);
end;

procedure TABCFileParser.ABCStructureToMemo(aMemo: TMemo);
begin
  if aMemo = nil then
    Exit;

  aMemo.Clear;
  aMemo.Lines.AddStrings(FABCStructure[0].ListOfArguments);

  // Assign aMemo to local FMemo
  FMemo := aMemo;
  FMemo.OnDblClick := MemoDblClick;
end;

procedure TABCFileParser.MemoDblClick(Sender: TObject);
begin

  ShowMessage('from Class : Memo clicked = ' + TMemo(Sender).Name);
    // OR ?
  ShowMessage('from Class : local Memo clicked = ' + FMemo.Name);
end;

procedure TABCFileParser.ABCStructureToFile(const aFileName: string);
begin
  // Save ABC Structure to File
  // ...
end;

end.

 

 

And here is used on Main form:

 

unit uMain;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls, uParseABCFile;

type
  TfrmMain = class(TForm)
    Label1: TLabel;
    Edit1: TEdit;
    Button1: TButton;
    Memo1: TMemo;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
    FABCStructure: TABCFileParser;
  public
    { Public declarations }
  end;

var
  frmMain: TfrmMain;

implementation

{$R *.dfm}

procedure TfrmMain.Button1Click(Sender: TObject);
begin
  FABCStructure := TABCFileParser.Create;
  FABCStructure.ABCFileName := '...';
  FABCStructure.ParseFile;
  FABCStructure.ABCStructureToMemo(Memo1);
end;

end.

 

When project is in CLI mode, this is example of how I will use the class:

 

procedure ParseABCFileAndExportToFile(const aABCFileName, aExportFileName: string);
var vABCParser: TABCFileParser;
begin
  vABCParser := TABCFileParser.Create;
  try
    vABCParser.ABCFileName := aABCFileName;
    vABCParser.ParseFile;
    vABCParser.ABCStructureToFile(aExportFileName);
  finally
    vABCParser.Free;
  end;
end;

 

So, the main question:

1. There will be more controls (probably multiple Virtual Treeviews for different views of data) interacting with this class, and also more export options for CLI mode, is it better to split (subclass) into a class that will interact with user controls and class that will not, will just work in CLI mode? 

 

And 2 small questions:

2. I assign MemoDblClick to a control, so is it better to use Sender argument or local FMemo variable that is assigned to this control? Or it doesn't matter in this case?

3.  Since FABCStructure: TABCFileParser; is private variable of Main form, it doesn't need manual Free-ing, since it will be freed (destroyed) when form is destroyed, right?

 

Thank you for any feedback!

 

[FredS 138]

For this I would simply use a Class Helper to separate the UI logic.

[David Schwartz 426]

Whoa. Spaghetti AND meatballs. 😮

 

Do NOT mix UI elements with data classes!

 

I don't have time to unravel this mess for you.

 

I'd start with your basic "item", which seems to be something like a TStringlist plus a name (that record thing). This could be a TUPLE of some sort. Alternatively, you could use the first entry of the stringlist to contain its own name, simply to avoid the additional structure just to have a separate identifier for it. But it may not even be necessary ... hang on.

 

Your second level is you have a collection of these items. You use a TArray, but it could be a TList or even a TCollection. Whatever. It needs Add, Insert, Get, and Delete methods, and maybe a lookup of some kind (so you can find an "Item by name").

 

However, because of the simplicity of this, you could use one main TStringlist where the Strings[n] element is a 'name' and Objects[n] is a TStringlist. Then sl.IndexOf(some_name) -> n, and sl.Objects[n] -> the TStringlist (from your record).

 

Viola! No need for a record or even an array. Just one TStringlist.

 

Why use multiple structures when one will work fine for all needs?

 

Technically speaking, while you could get by with just a single TStringlist, I'd wrap it into a class just to provide a simpler and more appropriate interface for it (eg., so you don't know how it's actually implemented). Memo.Lines is of type TStrings, so you'd want methods like these to interface nicely with TMemos:

 

function TmyClass.GetItem( const nm : string ) : TStrings;
var n : integer;
begin
    Result := NIL;
    n := sl.IndexOf(nm);   // assums 'sl' is the name of the main TStringlist in this object
    if (n >= 0) then
        Result := TStrings( sl.Objects[n] )
end;

procedure TmyClass.SetItem( const nm : string; Value : TStrings );
var n : integer;
    sl2 : TStringlist;
begin
    sl2 := GetItem(nm);
    if Assigned(sl2) then
        sl2.Assign(Value)
    else
    begin
        sl2 := TStringlist.Create();
        sl2.Assign(Value);
        sl.AddObject( nm, sl2 );
    end;
end;

 

Then when you need to get an item's value to a Memo, you'd use:
 

    memo1.Lines.Assign( myClassObj.GetItem( some_name ) ); 
    // maybe do GetItem first to check for NIL before calling Assign in case Assign chokes if you give it NIL

 

and to save the memo into an item buffer, you'd use:

 

    myClassObj.SetItem( some_name, memo1.Lines );

See how you're completely hiding the implementation, while providing an interface that makes using it very simple?

 

And it knows nothing about UI elements, and yet works very simply with them.

 

The class TmyClass is actually very simple, as it just contains a TStringlist (named 'sl' above). (Not DERIVED from it, but CONTAINS it.) The constructor would create it, and the destructor would destroy it.

 

You'd have GetItem, SetItem, and maybe one or two other methods.

 

That's it.

 

 

Edited February 6, 2019 by David Schwartz

[Mike Torrettinni 198]

Aha, I thought only the public methods should be Get/Set, and internal class methods don't need to be like this.

[Mike Torrettinni 198]

@David Schwartz If I understand correctly, any kind of complex type ( in my case a record with TStringList) should be a class with all Get/Set (Insert, Add, Deleted...) methods?

[David Schwartz 426]

9 hours ago, Mike Torrettinni said:

@David Schwartz If I understand correctly, any kind of complex type ( in my case a record with TStringList) should be a class with all Get/Set (Insert, Add, Deleted...) methods?

It seems you've got some heuristics in your head that are confusing you. There are no such rules.

 

I suspect folks here could come up with a half-dozen different approaches. This is my approach. None of them is "the right approach" if they all work.

 

I didn't put in any properties just to keep things simple for you. But you're welcome to add them and hide the explicit Get/Set methods.

 

OOP has three legs: Encapsulation, Inheritance, polymorphism.

 

Virtually all OOP situations involve Encapsulation; the others are only used if/when needed.

 

All I've done is employ encapsulation. The others are not needed.

 

In your approach, your encapsulation was overly complex and you kept trying to introduce unnecessary inheritance and composition.

 

If you simply use a TStringlist, you don't need a class. But if you do that, all of your interactions require the client code to know exactly what your implementation is -- a TStringlist of TStringlists. Or an array of TUPLEs (or 2-member records).

 

By encapsulating your implementation inside of a class, you can present a simple interface to the clients and completely hide the implementation. It doesn't matter if it's just one string or an entire frigging database system in the cloud with two levels of fall-back in case of failure. The client just sees one simple interface. And if you change the implementation, you don't have to change the way any other code deals with this class. That's the whole goal.

[David Schwartz 426]

On 2/6/2019 at 3:13 PM, Mike Torrettinni said:

Aha, I thought only the public methods should be Get/Set, and internal class methods don't need to be like this.

I did not show the class definition on purpose. So that's only implied.

 

Why don't you try writing the class definition (interface part) from what I've shown here so far?

[Mike Torrettinni 198]

Here is a little changed structure, because each item has a little more info than just Name and TStringList, so I changed the structure.

I also change to output String for Memo.

 

so, the idea behind this is:

 

TABCStructureNode  is how ABC file is structured, like xml node elements. So this is basic item.

TABCFileParser is now a parser that holds content of ABC file in FABCStructure: TArray<TABCStructureNode>;.

 

Now I get a structure into Memo with : Memo1.Lines.Text := FABCStructure.GetStructureAsString; where FABCStructure is private variable of Main form.

 

unit uParseABCFile2;

interface

uses System.Classes, Vcl.StdCtrls, Vcl.Dialogs;

type

  TNodeArgument = record
    ArgName  : string;
    ArgValue : string;
  end;

  // File structure basic block
  TABCStructureNode = class
  private
    FNodeLevel  : Integer;
    FNodeParent : string;
    FNodeName   : string;
    FArguments  : TArray<TNodeArgument>;
  private
    function GetArgument(Index: integer): TNodeArgument;
    function GetArgumentCount: integer;
    function GetArgumentAsString(Index: Integer): string;
  public
    constructor Create;
    property NodeLevel: integer read FNodeLevel write FNodeLevel;
    property NodeParent: string read FNodeParent write FNodeParent;
    property NodeName: string read FNodeName write FNodeName;

    // Arguments
    property ArgumentCount: integer read GetArgumentCount;
    property Arguments[Index: Integer]: TNodeArgument read GetArgument;
    procedure AddArgument(Argument: TNodeArgument);
    procedure DeleteArgument(Index: Integer);
    property ArgumentAsString[Index: Integer]: string read GetArgumentAsString;
    // ...
  end;


  TABCFileParser = class
    private
        FABCFileName: string;
        // File structure
        FABCStructure: TArray<TABCStructureNode>;
    public
      constructor Create;
      property ABCFileName: string read FABCFileName write FABCFileName;
      procedure ParseFile;

      // File structure
      procedure AddNode(NodeItem: TABCStructureNode);

      // CORRECT METHOD FOR UI INTERFACE
      function GetStructureAsString: string;

      // Methods for non-UI mode
      procedure ABCStructureToFile(const aFileName: string);
  end;


implementation

{ TABCStructure }

constructor TABCStructureNode.Create;
begin
  // ???
end;

function TABCStructureNode.GetArgument(Index: integer): TNodeArgument;
begin
  Result := FArguments[Index];
end;

procedure TABCStructureNode.AddArgument(Argument: TNodeArgument);
begin
  FArguments := FArguments + [Argument];
end;

procedure TABCStructureNode.DeleteArgument(Index: Integer);
begin
  Delete(FArguments, Index, 1);
end;

function TABCStructureNode.GetArgumentCount: integer;
begin
  Result := Length(FArguments);
end;

function TABCStructureNode.GetArgumentAsString(Index: Integer): string;
begin
  Result := FArguments[Index].ArgName + ' ' + FArguments[Index].ArgValue;
end;



{ TABCFileParser }

constructor TABCFileParser.Create;
begin
  // add something, if needed in the future
end;


procedure TABCFileParser.ParseFile;
var vNewItem : TABCStructureNode;
    vNewArgument: TNodeArgument;
begin
  // Parse/Read ABC File - FABCFileName

  // demo data

  vNewArgument.ArgName  := 'Arg1';
  vNewArgument.ArgValue := 'ArgValue';

  vNewItem := TABCStructureNode.Create;
  try
    vNewItem.NodeLevel  := 0;
    vNewItem.NodeParent := 'Parent';
    vNewItem.NodeName   := 'ABC Node 1';
    vNewItem.AddArgument(vNewArgument);
    AddNode(vNewItem);
  finally
    //vNewItem.Free; - THIS DELETES JUST ADDED ITEM???
  end;
end;


procedure TABCFileParser.AddNode(NodeItem: TABCStructureNode);
begin
   FABCStructure := FABCStructure + [NodeItem];
end;

procedure TABCFileParser.ABCStructureToFile(const aFileName: string);
begin
  // Save ABC Structure to File
  // ...
end;

function TABCFileParser.GetStructureAsString: string;
var
  i,j: Integer;
begin
  Result := '';
  for i := Low(FABCStructure) to High(FABCStructure) do
  begin
    Result := FABCStructure[i].NodeName;
    Result := Result  + ' Arguments: ';
    for j := 0 to FABCStructure[i].GetArgumentCount - 1 do
      Result := Result + FABCStructure[i].GetArgumentAsString(j);
  end;
end;

end.

how does this look?

 

 

[David Schwartz 426]

That looks a LOT better!

 

I'd fill out the constructors. The purpose of a ctor is to initialize the state of an object. When it's a simple item like what you've got, there might be multiple ctors: one would take no arguments and return an empty object, but initialize the contents anyway just to be safe. The other(s) would offer different combinations of parameters to allow you to create and set values in one statement. Instead, you create an empty object that's not initialized, then assign values to it separately. While that's fine, it's extra coding that can be eliminated.

 

Also, you seem addicted to using TArray, even though the way you're using it is more like a TList. You never set the initial size anywhere, which appears to be unbounded. So why not use a TList instead?

 

In ParseFile, you don't need try...finally because you don't want to delete the object you just created b/c it's being added to a list, as your comment suggests. If you delete it, you'll end up with a list full of pointers to freed objects.

 

Consider this instead of that whole block: 

AddNode( TABCStructureNode.Create( 25, 'parent_of_25', 'contents of node 25' ) );

But, if you stare at that long enough, you start to think, "why not just do this instead?"

AddNode( 25, 'parent_of_25', 'contents of node 25' );

where the TABCStructureNode is created implicitly by AddNode

Edited February 8, 2019 by David Schwartz

[Mike Torrettinni 198]

18 minutes ago, David Schwartz said:

That looks a LOT better!

It's the little things that count, like a little praise I did good 🙂

 

Thank you! I made changes and added new Create ctor:   constructor Create(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>); overload;

 

I also  changed private to strict private in TABCStructureNode, since the other class was able to access all fields directly instead of properties - the Code completion was confusing by allowing me access to FNodeLevel instead of just NodeLevel property:

 

I also added AddNnode overload that calls new ctor, can you confirm this is OK -  to use TABCStructureNode.Create in:

 

procedure TABCFileParser.AddNode(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>);
begin
  FABCStructure := FABCStructure + [TABCStructureNode.Create(Level, Parent, NodeName, Arguments)];
end;

 

As for the TArray vs TList... I just like TArray, as I'm used to it.

 

unit uParseABCFile2;

interface

uses System.Classes, Vcl.StdCtrls, Vcl.Dialogs;

type

  TNodeArgument = record
    ArgName  : string;
    ArgValue : string;
  end;

  // File structure basic block
  TABCStructureNode = class
  strict private
    FNodeLevel  : Integer;
    FNodeParent : string;
    FNodeName   : string;
    FArguments  : TArray<TNodeArgument>;
  strict private
    function GetArgument(Index: integer): TNodeArgument;
    function GetArgumentCount: integer;
    function GetArgumentAsString(Index: Integer): string;
  public
    constructor Create; overload;
    constructor Create(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>); overload;
    property NodeLevel: integer read FNodeLevel write FNodeLevel;
    property NodeParent: string read FNodeParent write FNodeParent;
    property NodeName: string read FNodeName write FNodeName;

    // Arguments
    property ArgumentCount: integer read GetArgumentCount;
    property Arguments[Index: Integer]: TNodeArgument read GetArgument;
    procedure AddArgument(Argument: TNodeArgument);
    procedure DeleteArgument(Index: Integer);
    property ArgumentAsString[Index: Integer]: string read GetArgumentAsString;
    // ...
  end;


  TABCFileParser = class
    private
        FABCFileName: string;
        // File structure
        FABCStructure: TArray<TABCStructureNode>;
    public
      constructor Create;
      property ABCFileName: string read FABCFileName write FABCFileName;
      procedure ParseFile;

      // File structure
      procedure AddNode(NodeItem: TABCStructureNode); overload;
      procedure AddNode(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>); overload;

      // CORRECT METHOD FOR UI INTERFACE
      function GetStructureAsString: string;

      // Methods for non-UI mode
      procedure ABCStructureToFile(const aFileName: string);
  end;


implementation

{ TABCStructure }

constructor TABCStructureNode.Create;
begin
  // ???
end;

constructor TABCStructureNode.Create(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>);
begin
  FNodeLevel  := Level;
  FNodeParent := Parent;
  FNodeName   := NodeName;
  FArguments  := Arguments;
end;

function TABCStructureNode.GetArgument(Index: integer): TNodeArgument;
begin
  Result := FArguments[Index];
end;

procedure TABCStructureNode.AddArgument(Argument: TNodeArgument);
begin
  FArguments := FArguments + [Argument];
end;

procedure TABCStructureNode.DeleteArgument(Index: Integer);
begin
  Delete(FArguments, Index, 1);
end;

function TABCStructureNode.GetArgumentCount: integer;
begin
  Result := Length(FArguments);
end;

function TABCStructureNode.GetArgumentAsString(Index: Integer): string;
begin
  Result := FArguments[Index].ArgName + ' = ' + FArguments[Index].ArgValue;
end;



{ TABCFileParser }

constructor TABCFileParser.Create;
begin
  // add something, if needed in the future
end;


procedure TABCFileParser.ParseFile;
var vNewItem : TABCStructureNode;
    vNewArgument1, vNewArgument2: TNodeArgument;
    vArguments1, vArguments2: TArray<TNodeArgument>;
begin
  // Parse/Read ABC File - FABCFileName

  // demo data
  vNewArgument1.ArgName := 'Arg1'; vNewArgument1.ArgValue := 'Value1';
  vNewArgument2.ArgName := 'Arg2'; vNewArgument2.ArgValue := 'Value2';
  vArguments1 := TArray<TNodeArgument>.Create(vNewArgument1, vNewArgument2);

  vNewArgument1.ArgName := 'Arg3'; vNewArgument1.ArgValue := 'Value3';
  vNewArgument2.ArgName := 'Arg4'; vNewArgument2.ArgValue := 'Value4';
  vArguments2 := TArray<TNodeArgument>.Create(vNewArgument1, vNewArgument2);


  AddNode(0, 'Parent',     'ABC Node 1', vArguments1);
  AddNode(1, 'ABC Node 1', 'ABC Node 2', vArguments2);
end;


procedure TABCFileParser.AddNode(NodeItem: TABCStructureNode);
begin
   FABCStructure := FABCStructure + [NodeItem];
end;

procedure TABCFileParser.AddNode(Level: integer; const Parent, NodeName: string; const Arguments: TArray<TNodeArgument>);
begin
  FABCStructure := FABCStructure + [TABCStructureNode.Create(Level, Parent, NodeName, Arguments)];
end;

procedure TABCFileParser.ABCStructureToFile(const aFileName: string);
begin
  // Save ABC Structure to File
  // ...
end;

function TABCFileParser.GetStructureAsString: string;
var
  i,j: Integer;
begin
  Result := '';
  for i := Low(FABCStructure) to High(FABCStructure) do
  begin
    Result := Result  + FABCStructure[i].NodeName;
    Result := Result  + ' Arguments: ';
    for j := 0 to FABCStructure[i].ArgumentCount - 1 do
      Result := Result + FABCStructure[i].ArgumentAsString[j] + '; ';
    Result := Result + sLineBreak;
  end;
end;

end.

 

 

[David Schwartz 426]

Better.

 

Now, refactor this so the implementation of Arguments is not visible anywhere other than inside of a constructor somewhere.

 

Again, the whole idea of encapsulation is to hide the implementation of data so it can change without affecting how it's used by clients.

 

IOW, if you were to change it from TArray to TList or TDictionary, none of your application code would be affected.

[Mike Torrettinni 198]

OK, thanks for all the time and effort helping me!