Is Class with 2 'nested' constructors bad design?

[Mike Torrettinni 198]

I have 2 constructors in my class and Create2WayCompare calls default Create constructor:

TINIComparer = class
public
    constructor Create(const aINIFileName1, aINIFileName2: string); overload;
    constructor Create2WayCompare(const aINIFileName1, aINIFileName2: string); overload;
end;

constructor TINIComparer.Create2WayCompare(const aINIFileName1, aINIFileName2: string);
begin
  fCompareType := cmt2WayCompare;
  Create(aINIFileName1, aINIFileName2);  
end;

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

  LoadINIFiles;
  CompareINIFiles;
end;

// from main form:
fINIComparer := TINIComparer.Create2WayCompare(edtINIFilename1.Text, edtINIFilename2.Text);

I'm planning to have a few more Create???() constructors that will indicate different class options.

 

This works good, no memory leaks no AV errors. But 2 constructors are executed. Is this OK, just bad design or something is wrong but it didn't show up yet with such a simple class example?

 

 

 

[Attila Kovacs 629]

as long as nobody else have to use this class and you have memories like an elephant, it should be fine..

[Mike Torrettinni 198]

@Attila Kovacs Is my question so bad, you can't give me a little more details? 

[Attila Kovacs 629]

@Mike Torrettinni It's completely fine, I'm just against "Indian Naming". What if your class gets 2 more options, how would you name the constructors?

[Kryvich 165]

While it's OK, I'd suggest to keep constructors as simple as possible. To prevent exceptions while an object is created. 

[David Heffernan 2345]

It works. But better to have one constructor that accepts an argument. Think of the time you need to decide at runtime which path to take. Then your way is hopeless. 

[Mike Torrettinni 198]

Thanks!

So, if compare options are:  TCompareType = (cmt2WayCompare, cmt3WayCompare, cmtHeadersOnly, cmt3WayMerge ...);

I could have: constructor Create( compare option, Ini1, Ini2) but this means that main form (or who ever will use the class) has to use the unit where TCompareType is defined.

So, I wanted to avoid with with just creating multiple constructors and the main form doesn't need to know the options, just the name of constructor should be OK.

 

constructor Create( compare option, Ini1, Ini2)

or

constructor Create2WayCompare(Ini1, Ini2)

constructor Create3WayCompare(Ini1, Ini2)

constructor Create3WayMerge(Ini1, Ini2)

...

 

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

Edited March 13, 2020 by Mike Torrettinni

[Attila Kovacs 629]

constructor Create(const AIniFileName1, AIniFileName1: string; AOptions: TCompareOptions = []);

 

Where else would you declare TCompareOptions as in the same unit as this class is?

Edited March 13, 2020 by Attila Kovacs

[Mike Torrettinni 198]

7 minutes ago, Attila Kovacs said:

Where else would you declare TCompareOptions as in the same unit as this class is?

No, I want to hide the TCompareOption from Form1 and other units that will use TINIComparer class. Of course class unit has to know the options.

[Attila Kovacs 629]

First you said it's in a different unit, now you are telling me, you want to hide it. I'm not sure what your goal is, why would you have it in a different unit or why would you want to hide it? There is something else you are not telling or you are not sure about. Lay your cards on the table.

[Mike Torrettinni 198]

Perhaps I wasn't focused on the real problem:

 

unit uINIComparerStructure has TCompareType + other INI related types.

unit uINIComparer has TINIComparer class; and uses uINIComparerStructure unit.

(other units also use uINIComparerStructure since they deal with INI content as needed)

 

Form1 uses uINIComparer unit to use TINIComparer class.

 

I would like to hide uINIComparerStructure unit from Form1, so I don't expose all types that Form1 doesn't need to know about.

 

 

[Attila Kovacs 629]

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.

[Lars Fosdal 1792]

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)

 

Edited March 13, 2020 by Lars Fosdal
Fixed a typo in source example

[David Heffernan 2345]

49 minutes ago, 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. 

Edited March 13, 2020 by David Heffernan

[David Heffernan 2345]

1 minute ago, Lars Fosdal said:

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

Also a false goal. 

[Lars Fosdal 1792]

Just now, David Heffernan said:

Also a false goal. 

I like streamable objects. That makes it impractical with params to the constructor. Hence, I try to avoid them.

[Mike Torrettinni 198]

Just now, David Heffernan said:

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

My idea was to design is as closed unit as possible, so it can be used by Form for visual presentation, as background service to export results and so on. Perhaps a little overkill.

[Remy Lebeau 1394]

2 hours ago, Mike Torrettinni said:

I have 2 constructors in my class and Create2WayCompare calls default Create constructor

I hope your class never gets used in C++, because the resulting constructors would have the exact same names and parameter lists, and thus be ambiguous and unusable!

 

[Mike Torrettinni 198]

54 minutes ago, 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)

 

Thanks, so each type of comparer would be its own class, derived from main class. OK, not something I considered.

[David Heffernan 2345]

59 minutes ago, Mike Torrettinni said:

Thanks, so each type of comparer would be its own class, derived from main class. OK, not something I considered.

OMG, take a poor idea, and make it much worse

[Clément 148]

I like my constructors to construct 
No loading, no process, no fancy stuff inside the constructor other than parameter assignment, variable initialization or other class creation.

 

[Lars Fosdal 1792]

4 hours ago, David Heffernan said:

OMG, take a poor idea, and make it much worse

Please elaborate. How is it worse?

How would you solve it? 

[Andrea Raimondi 13]

Use nested, private classes and expose the whole thing via proper methods. 

[David Heffernan 2345]

6 hours ago, Lars Fosdal said:

How is it worse?

You are advocating defining new classes in order to set a single field in the base class. If you can't see what is wrong with that I am surprised. 

 

6 hours ago, Lars Fosdal said:

How would you solve it? 

I already said. 

[Lars Fosdal 1792]

The point of making a second class was to enable one of the features that Mike was looking for, i.e. hiding implementation details. 
The single set field was a single property of the single example Mike initially gave.  He later pointed out there are at least four different methods of operation, even adding three-way comparison/merges, meaning you need a third file parameter. 
You can put these classes in separate units and have a factory method or class that instantiate the specific inner classes, and completely isolate the inner workings from the outer use, offering f.x. two compare methods.

function Compare(const aFileName1, aFileName2: string; const aMerge: Boolean = False):TResultType; overload;
function Compare(const aFileName1, aFileName2, aFileName3: string; const aMerge: Boolean = False):TResultType; overload;

Again, just a simple example based on the limited insight into all the possible variations of parameterization.
 

There is more than one way to solve such a challenge.
I chose to use polymorphism and encapsulation.