Jump to content
Mike Torrettinni

Is Class with 2 'nested' constructors bad design?

Recommended Posts

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?

 

 

 

Share this post


Link to post

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

  • Like 1
  • Confused 1

Share this post


Link to post

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

Share this post


Link to post

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. 

  • Like 1

Share this post


Link to post

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 by Mike Torrettinni

Share this post


Link to post

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

 

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

Edited by Attila Kovacs
  • Like 1

Share this post


Link to post
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.

Share this post


Link to post

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.

Share this post


Link to post

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.

 

 

Share this post


Link to post

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.

  • Like 1

Share this post


Link to post

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 by Lars Fosdal
Fixed a typo in source example
  • Like 1
  • Thanks 1

Share this post


Link to post
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 by David Heffernan
  • Like 1
  • Thanks 1

Share this post


Link to post
1 minute ago, Lars Fosdal said:

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

Also a false goal. 

  • Like 1

Share this post


Link to post
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.

Share this post


Link to post
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.

Share this post


Link to post
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!

 

Share this post


Link to post
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.

Share this post


Link to post
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

  • Haha 1

Share this post


Link to post

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

 

  • Like 1
  • Thanks 1

Share this post


Link to post
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? 

Share this post


Link to post
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. 

Share this post


Link to post

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.

  • Thanks 1

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×