Is Class with 2 'nested' constructors bad design?

[Mike Torrettinni 198]

23 hours ago, Attila Kovacs said:

Ok. Try to restructure your units until TINIComparer and TCompareType are in the same unit, and don't be shy creating 2-3 more units if needed.

Unseen, i would say, your structure is done when "uINIComparerStructure" becomes empty. 

Thanks, the reason for separate uINIComparerStructure is that many other units will do the 'work', like I have visual presentation class, TPresentationLink,  that links VirtualStringTree and compare results - in this case TPresentationLink unit has to know the basic types.

TINIComparer = class
...
  fPresentationLink: TPresentationLink; // in uINIDataLink
  procedure AssignPresentationLink(aVST1, aVST2: TObject);

procedure TINIComparer.AssignPresentationLink(aVST1, aVST2: TObject);
begin
  fPresentationLink := TPresentationLink.Create(aVST1, aVST2, GetData1, GetData2);
end;

 

So, in Form1 I set fINIComparer.AssignPresentationLink(vstINI1, vstINI2); and have access to results in VSTs.

 

The naming of units will change with project evolving.

Edited March 14, 2020 by Mike Torrettinni

[David Schwartz 426]

The purpose of a class constructor is to initialize the class to a known initial state when it's created. Imagine having to different ways to start your car if you were alone or if you had passengers, just so the system that supports airbags knows whether to only activate the Driver's-side airbag or all of them.

 

Airbag activation based on secondary properties that can't be known until run-time (ie, how many people get into the car) is not part of the object's "initial state".

 

FIRST you start the car. THEN you do other things like activate the airbags.

 

You do NOT want to have different ways of starting the car based on factors that depend upon secondary systems.

 

It doesn't need to be airbags -- it could be A/C vs. Heater; driving in the day vs. at night where lights are required; etc.

 

Creating your objects is like starting the car and getting the engine running. 

 

C++ and C# (among others) have a way to inject initial state data into constructors at the time of declaration vs. run-time. Delphi doesn't have that ability, so you need to inject the really core initial parameters in the Create call, and then add others AFTER the object has been initialized via property injection.

[Mike Torrettinni 198]

Thanks for all the feedback! Single constructor Create from now on 🙂

[David Heffernan 2345]

1 hour ago, David Schwartz said:

Delphi doesn't have that ability, so you need to inject the really core initial parameters in the Create call, and then add others AFTER the object has been initialized via property injection.

This seems bogus. Define "really core" please. 

[David Schwartz 426]

1 hour ago, David Heffernan said:

This seems bogus. Define "really core" please. 

perhaps it's a poor choice of words. 

 

Usually one would construct an object, and when it's fully constructed, it can be used. Values can be injected via the ctor's Create parameter list.

 

Sometimes this isn't possible for various reasons. In that case, Create would build as much as it can, then you'd use property or method injection to initialize subsequent values, then proceed to work with the object.

 

The benefit of injecting values in the Create parameter list is that once the Create finishes, you're supposed to have confidence that it has been properly constructed if it doesn't throw an exception.

 

When you use property or method injection to perform further required aspects of initializing the object, that's not necessarily the case, as there would be one or more methods in the class that would not work properly if these further initialization operations are not done first.

 

This is distinct from simply setting state variables in the object to alter its run-time operation. 

 

Here's an example that could go either way, but take it in the spirit it's being offered...

 

When you start your car (ie, initialize it), it's ready to go. You can put it in gear and just drive off.

 

But there's a step that most older cars require that newer cars don't, which is "check need for headlights".

 

If it's dark outside, you can drive the car away without turning on your headlights, but you risk running into something, or getting a ticket. Said another way, there's a high potential for a run-time exception to occur if the lights aren't properly initialized.

 

So there's a secondary initialization needed where the driver needs to make an observation and then turn the headlights on before driving away if it's sufficiently dark outside.

 

Newer cars have put sensors in the vehicle that automatically turn on the headlights when the ambient lighting drops below some predefined level. So this way, no further action is required -- you can indeed get into the car, start it, and then drive off without having to explicitly check the headlights since it's now part of the "core" initialization functions.

 

The headlight setting in this case will not prevent the car from driving, but it can lead to a harmful situation if ignored.

 

(In my car, I have to depress the brake pedal in order to initialize (start) the car, so it's an implicit part of the initialization sequence.)

 

Another one is if you're low on fuel. The car won't start if you're out of gas, but if you're merely "low" it will start and run for a while, then die. If you know you need to drive 100 miles, but there's no enough gas to go that far, then you're going to get a run-time exception if you don't increase the fuel level. Not much different than running out of memory after awhile.

 

It's left as an exercise for the driver to check the fuel level directly and add gas when needed.

 

In the 1980's, cars had a "nag" feature built-in that would tell you things like "fasten your seat belt," "check gas level," or "door is ajar". They lasted longer than anybody imagined, but this feature was thoroughly despised by most drivers. This was a "warning" rather than an "error", so the car would drive after being started, but the "nagging" announcements were intended to alert the driver to things they presumably weren't aware of. 

 

An "out of gas" state is the only one I can think of when the car won't start (initialize). 

 

The transmission setting is distinctly NOT part of the initialization sequence, because it's something that changes the state of the vehicle during normal run-time operations.

[David Heffernan 2345]

This class can't be used without the compare type being specified. 

[Sherlock 663]

I have the strong feeling, that putting this much thought into that seemingly simple class will lead to "What the hell was I thinking?" sometime in the not so far future. KISS really is my prime directive when it come to software development.

[Fr0sT.Brutal 899]

15 hours ago, David Schwartz said:

The purpose of a class constructor is to initialize the class to a known initial state when it's created. Imagine having to different ways to start your car if you were alone or if you had passengers, just so the system that supports airbags knows whether to only activate the Driver's-side airbag or all of them.

If a class requires pretty much parameters being set to operate correctly, there's not much to do but call constr with some params and then init the object some more before running its methods. After turning on ignition, driver has to release handbrake and switch to drive mode anyway.

Alternative is using a record with full set of required values as constr parameter but it causes some other gotchas.

Edited March 17, 2020 by Fr0sT.Brutal

[David Schwartz 426]

9 hours ago, Fr0sT.Brutal said:

If a class requires pretty much parameters being set to operate correctly, there's not much to do but call constr with some params and then init the object some more before running its methods. After turning on ignition, driver has to release handbrake and switch to drive mode anyway.

Alternative is using a record with full set of required values as constr parameter but it causes some other gotchas.

In my mind, "initializing the car" is mostly focused on getting the engine started and ready to drive. There are all sorts of pre-checks that could be performed, like pilots do on aircraft: tire pressure, chalk-blocks on tires, garage door, main gate, parking brake, fluid levels and brakes, lights, windows, A/C or heat, defroster, etc. So to a certain extent, the analogy breaks down. Personally, I'd say the "base class" initialization is just what's going on "under the hood" and everything else delegated to a derived class, or the driver, depending on how much of it is automated. You say po-TAY-to and I say po-TAA-to. It's not worth arguing over. 🙂

[Mike Torrettinni 198]

On 3/13/2020 at 1:23 PM, Lars Fosdal said:

Having arguments to the constructor is something I try to avoid.

TINIComparer = class
private
  fINIFileName1: string;
  fINIFileName2: string;
protected
  procedure LoadINIFiles;
  procedure CompareINIFiles; virtual;

public
  constructor Create; virtual;
  procedure Compare(const aINIFileName1, aINIFileName2: string); 
end;

TIniComparer2Way = Class(TIniComparer)
protected
  procedure CompareINIFiles; override; // if necessary
public
  constructor Create; override;
end;


constructor TINIComparer.Create;
begin
  Inherited;
end;

constructor TINIComparer.Compare(const aINIFileName1, aINIFileName2: string);
begin  
  fINIFileName1 := aINIFileName1;
  fINIFileName2 := aINIFileName2;

  LoadINIFiles;
  CompareINIFiles;
end;

constructor TINIComparer2Way.Create;
begin
  Inherited;
  fCompareType := cmt2WayCompare;
end;


// from main form:
fINIComparer := TINIComparer2Way.Create;
fIniComparer.Compare(edtINIFilename1.Text, edtINIFilename2.Text)

 

@Lars Fosdal, thank s for the original idea, but I have a dilemma here what to do: if I add a 3WayCompare:

 

constructor TINIComparer3Way.Create;
begin
  Inherited;
  fCompareType := cmt3WayCompare;
end;

Now the main class will do all the work based on fCompareType , all the comparison, preparing results for visual display, reporting and such - so a lot of:

if fCompareType = cmt2WayCompare then ... 
else if  fCompareType = cmt3WayCompare then ...

All good, makes sense.

 

But now we could say that the base class implements the details of sub class. Isn't this wrong approach, shouldn't base class 'not know' anything about the implementation of it's sub classes? If base class should be oblivious of sub classes, this means I need to implement everything in sub classes where all the methods are very similar, just slight difference.

 

But when everything is implemented in sub classes, except for only 1 or 2 common methods stay in base class, so why the need for base class, at all?

 

Same goes for presentation layer: if I want to display/export/report 3WayCompare with just 1 record field more than 2WayCompare, it's again a lot of IFs in 1 presentation class, or 2 completely separate presentation classes with their own methods.

 

The more I read about class inheritance, the more confused I get, because examples like TAnimal <- TDog, TCat makes absolute sense. but when you need a little more implementation details, it doesn't make sense at all.

 

Any feedback is appreciated!

 

Edited April 1, 2020 by Mike Torrettinni

[Lars Fosdal 1789]

It requires some practice to wrap your head around encapsulation (pun intended) and thinking about how to make your code general or generic, if you like.

A common base class mostly makes sense if you want to be able to ignore how it is used later, i.e. rely on polymorphism, and when you have a certain degree of reuse of code.

Specific: Instance := TClassType.Create, then provide params -- Ideally, this would be the one place where you do something specific for the type when using it.
General. Instance.LoadFiles;
General: Instance.Compare;
General: Output := Instance.CreateOutput;
General: Render  Output 

As you expand your hierarchy, you add properties and features only on the level where you need them.

type 
  TCompare = class
   procedure LoadFiles; virtual;
   procedure Compare; virtual;
   property File1: string;
   property File2: string;
  end;

  TCompare2Way = class(TCompare)
    procedure Compare; override;
    property Merge: Boolean;
  end;

  TCompare3Way = class(TCompare2Way)
    procedure LoadFiles; override;
    procedure CompareLeft;
    procedure CompareRight;
    procedure Compare; override;
    property File3: string;
end;

 

[Lars Fosdal 1789]

An alternative is to create completely independent classes, and use an interface to define the methods that you want to be general across the classes.

[Mike Torrettinni 198]

29 minutes ago, Lars Fosdal said:

It requires some practice to wrap your head around encapsulation (pun intended) and thinking about how to make your code general or generic, if you like.

A common base class mostly makes sense if you want to be able to ignore how it is used later, i.e. rely on polymorphism, and when you have a certain degree of reuse of code.

Specific: Instance := TClassType.Create, then provide params -- Ideally, this would be the one place where you do something specific for the type when using it.
General. Instance.LoadFiles;
General: Instance.Compare;
General: Output := Instance.CreateOutput;
General: Render  Output 

As you expand your hierarchy, you add properties and features only on the level where you need them.

type 
  TCompare = class
   procedure LoadFiles; virtual;
   procedure Compare; virtual;
   property File1: string;
   property File2: string;
  end;

  TCompare2Way = class(TCompare)
    procedure Compare; override;
    property Merge: Boolean;
  end;

  TCompare3Way = class(TCompare2Way)
    procedure LoadFiles; override;
    procedure CompareLeft;
    procedure CompareRight;
    procedure Compare; override;
    property File3: string;
end;

 

So I have 2 sub classes with a lot of methods, that are defined as virtual or virtual abstract methods in base class. But this is just a waste of time to define them in base class, when they need to be overriden in sub class, too.

At this moment, the only real information completely independent of sub classes are: Error messages, IsLoadingDone, IsComparingDone, NoOfDifferences, File1, File2 and such... basically nothing of real value that would warrant a base class.

 

I just started with classes, please don't suggest Interfaces 🙂

It makes much more sense to have 2 separate classes and common utility methods that do the work based on fCompareType parameter. This makes sense.

 

 

[David Schwartz 426]

This does not smell right to me.

 

You're using inheritance to model composition, IMO.

 

Your base object is a file. You don't really need to make a class around just a file. 

 

If you want to operate on two files, you don't do that by inheritance -- that's composition.

 

You'd want a function that takes two files and compares them and returns some kind of result. You don't need a class for this either.

 

Another function could take one input file and the diff data and regenerate the second file. 

 

In a typical Linux command shell, you'd use:

 

diff -e file1 file2 >diff_f1_f2

 

You also have directionality, so you could go the other way:

 

diff -e file2 file 1, >diff_f2_f1

 

These output files are called "deltas" and one is a "forward delta" while the other is a "reverse delta". 

 

The "diff -e" command gives you an output file that consists of a series of edit commands (for 'ed') that you can feed into it with the first file to get at the second file.

 

ed -f diff_f1_f2 file1 > file2_again

 

I'm hard-pressed to think how I'd build classes around these commands.

 

You could have a file mirroring scheme that uses a LocalStore and a RemoteStore. Each store would keep reverse deltas, but the LocalStore would generate forward deltas to send to the RemoteStore to reflect the latest changes to a file without having to send the entire file. This makes sense -- this scheme is maintaining state by keeping the RemoteStore an accurate reflection of the LocalStore while minimizing transmission bandwidth.

 

If all you're trying to do is visualize things, that's a little different, but you could think of the output as commands to colorize the text rather than edit it. You'd go one direction then the other and colorize each file based on changes needed to get one side from the other. But you're still not maintaining any kind of state, since the files are static and everything can be derived again at any time from the same input files. The FILES themselves represent the "current state" of their relationship that you're looking to visualize.

 

The 3-way compare simply uses three files and does three (or six) different comparisons.

 

A class implements behavior and maintains state. There's no state that needs to be maintained here -- just some functions that take input files and generate output files. The same output is produced for the same input, every time. 

 

One could argue that the functions implement behavior -- yes, but they don't change the state of the underlying data. So it's just a container of convenience.

 

This seems like a rather far reach to come up with a class design when it's of no obvious benefit.

Edited April 2, 2020 by David Schwartz

[Mike Torrettinni 198]

10 hours ago, David Schwartz said:

A class implements behavior and maintains state. There's no state that needs to be maintained here -- just some functions that take input files and generate output files. The same output is produced for the same input, every time. 

Thank you for detailed explanation, I guess I'm trying to fit something into a class hierarchy, that is not designed for it. The class inheritance looked pretty good, because I'm dealing with a few different file types, very close to INI, but customized.

I was thinking to create a base class and then sub classes for each file type, but they are so different in content, purpose and visual presentation - then there is comparison representation, that is unique to each of them. So, ti was annoying I needed to override 90% of the methods of the base class - it doesn't make sense to have base class at all, then.

 

[David Heffernan 2345]

Sounds like the classic OOP trap of trying to solve all problems with inheritance. 

 

Good OOP programmers hardly ever use inheritance to solve problems. 

[Mike Torrettinni 198]

2 minutes ago, David Heffernan said:

Good OOP programmers

It's on a horizon... still a little to go 🙂

[David Schwartz 426]

One of the biggest challenges people new to OOP run into is they don't really grasp that inheritance and composition are two equally useful tools, except they solve different problems. People tend to over-do the use of inheritance at first. I guess it just seems cool. Most of the time, all you need is, say, three classes instead of one, and then you compose objects from those classes into something else. As opposed to some contorted mess created from a strange hierarchy of inherited classes that you think might do the same job.

 

I like to think of inheritance as just another form of Dependency Injection -- one that injects things into a class via ancestry rather than constructor, property, or method injection. You can inject behavior (via methods in the base class) as well as state (via common state variables in the base class). If you only need to inject patterns of behavior (like frameworks with no default behavior) but no state, then use Interfaces.

 

Sometimes it can be rather fuzzy which way to go. I think the most common class I spend too much time deliberating over is the lowly TStringList. Sometimes I'll need something a little off the grid and add functions to a stringgrid. Then I'll put them all into their own class (encapsulation). Then I'll scratch my head and wonder if the stringlist should be a member of the class (composition) or if I should actually inherit from it (inheritance). It's not always obvious, and sometimes it's even only worth a coin-toss. 

 

Ditto when it comes to any kind of container -- do I inherit from a TList<something> or do I compose the TList inside of the class and reflect the methods I need and skip the rest? 

 

Either way, it's necessary to create methods to reflect the things I want to do that aren't already there. A common example is adding a new widget to a list of widgets. Inheritance lets you use an inherited Add method for free, while composition requires you to implement one yourself. But sooner or later you're going to think ... why not add a AddNewWidget( ... ) function that returns a TWidget to the container class so I don't have to create a new one first then add it separately. Oh, then I can throw in some parameters that I'll usually initialize it with, sort of like a factory pattern. That AddNewWidget function must be created yourself whether you use inheritance or composition to manage the list. And there are others as well. So it's not as simple as just looking at the built-in methods. (Yes, people get obsessed wtih not having to create reflection methods like Add, Delete, and a search function when they use composition rather than inheritance.)

 

But over time, I'd have to say I don't spend time going around looking for ways to use inheritance. That's like carrying around an umbrella on a sunny day looking for rain. 

 

Rather, I wait until I run into a situation where I've got two or three classes that CLEARLY share some common behavior and/or states, and then I'll pull the common parts into a base class that I'll then inherit from.

If you're trying to figure out how to accomplish some kind of inheritance, then I'd say you're not being driven by the nature of the problem, but mostly by your desire to fit a large square peg into a small round hole. The question is not, "How do you do this?" but "Why are you even trying?" The "how" part should really should be obvious, IMHO.

Edited April 3, 2020 by David Schwartz

[Mike Torrettinni 198]

Thank you, I think you very clearly explained that I need experience. I'm on my first steps with classes, the more I use them the more experience I will have, the more I will know what to do - I'm not even close to your CLEARLY.

I can read 100s of articles on Inheritance over composition, still practice is something else. For now, I'm in a phase of making mistakes 🙂 

[Mike Torrettinni 198]

 

On 3/13/2020 at 1:25 PM, David Heffernan said:

On 3/13/2020 at 12:35 PM, Mike Torrettinni said:

So, the whole point was use as little units as possible.

That's a false goal. Remove that goal from your life. Following it will be making your code worse. 

 

I knew somebody already told me this, but I'm glad I found it to stop me going down that path again 🙂

 

 

[dummzeuch 1499]

On 4/3/2020 at 12:21 PM, Mike Torrettinni said:

For now, I'm in a phase of making mistakes 🙂 

You will never leave that phase, just the number of mistakes will shrink over time. At least that's how it has been with me. But on the bright side: Making mistakes - and recognising them - is a great way of learning.

[Stano 143]

In my opinion, the only one. And always and everywhere.