There are two ways to implement overloads. The first one is to do everything in one method/constructor and call it from other overloads, which leads to longer method bodies. The second one is to do the minimum in each overload, thus having a code sometimes difficult to navigate and to understand which overload does what.
For example, if two overloads of a class Cat
are:
public Cat(string name, int? weight, Color mainColor);
public Cat(string name);
there are two ways to implement this:
public Cat(string name, int? weight, Color mainColor)
{
// Initialize everything.
this.name = name;
if (weight.HasValue) this.weight = weight.Value;
// There is a bug here (see the anwer of @Timwi): mainColor can be null.
this.colors = new List<Colors>(new[] { mainColor });
}
public Cat(string name)
: this(name, null, null)
{
// Nothing else to do: everything is done in the overload.
}
public Cat(string name)
{
// Initialize the minimum.
this.name = name;
this.colors = new List<Colors>();
}
public Cat(string name, int? weight, Color mainColor)
: this(name)
{
// Do the remaining work, not done in the overload.
if (weight.HasValue) this.weight = weight.Value;
this.colors.Add(mainColor);
}
Note: since C# 4.0 let you specify optional parameters, to avoid ambiguity, let's say I'm talking about C# 3.0 only.
I think this is another one of those examples where no single, dogmatic answer will reasonably cover all cases. I would always look at the individual case and decide based on all available factors.
One factor is that the first one has lots of ifs. Your code also has a bug: you would be adding a null
value into the list of colors; in order to fix that bug, you need even more ifs. Such a constructor can easily get messy. A myriad of ifs is indicative that there are several cases in which the logic is substantially different, so having separate constructors for each of those cases makes perfect sense.
However, in cases where there aren’t that many ifs, the logic is the same for all, so now it makes sense to call a single constructor that does that one logic, and does it well. Then there’s only one place to maintain it.
Another factor is that in your example, the first one leaves weight
uninitialised. This need not be a bad thing because fortunately default initialisation in C# is predictable; but I would consider it bad form if the field declaration for weight
were to initialise it to something non-zero and only some of the constructors overwrite that default with another value. The constructor parameters and/or the this(...)
call are better places to document the default value for that field. (Preferably the constructor parameters, because then even the client programmer can see the default value, but obviously that requires C# 4.) Of course, it is also possible to initialise all the fields using a field initialiser and leave the constructor(s) empty, which is a reasonable strategy if you only have one constructor with no arguments.
So yeah, like you said, you don’t want method bodies to get too long, but you also don’t want the code to get too difficult to navigate, so you need to strike a balance between the two for any given situation.
First one
Constructor overload must always call each other or a common initializer method. It is a concern of refactoring and code modularity so that you would have to make a change only once.
For example if you want to add this:
this.name = name ?? string.Empty;
In the second case you have to do it in two places for the second one but in one place in the first one.
Also per convention, order the constructors such that they start with the least parameters first.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With