32

Is it a good idea (from a design POV) to nest constructor calls for overloaded New or Factory style methods? This is mostly for simple constructors, where each overload builds on the previous one. 

MyClass( arg1 ) { 
    _arg1 = arg1; 
    _otherField = true; 
    _color="Blue" 
}
MyClass( arg1, arg2) : this(arg1) { 
    _arg2 = arg2  
}
MyClass( arg1, arg2, arg3) : this(arg1, ar2) { 
    _arg3 = arg3; 
}

Or with factory methods:

static NewInstance(arg1 ) { 
   _arg1 = arg1;       
}
static NewInstance(arg1, arg2) {
   f = NewInstance(arg1);
   f._arg2 = arg2;
}
//... and so on

I can see a few drawbacks on both sides

  • Nesting hides what the constructor is doing
  • Not nesting duplicates all the functionality

So, is doing this a good idea, or does it set me up for something I'm just not seeing as a problem. For some reason I feel uneasy doing it, mostly because it divides up the responsibility for initializing.

Edit: @Jon Skeet: I see now why this was bothering me so much. I was doing it backwards! I wrote the whole thing and didn't even notice, it just smelled. Most other cases I have (that I wrote), do it the way you recommend, but this certainly isn't the only one that I have done like this. I do notice that the more complicated ones I did properly, but the simple ones I seem to have gone sloppy. I love micro edits. I also like acronymns!

Andrew
  • 8,322
  • 2
  • 47
  • 70

2 Answers2

63

I think it's reasonable to chain constructors together, but I do it the other way - the version with fewer parameters calls the version with more parameters. That way it makes it very clear what's happening, and all the real "logic" (beyond the default values) is in a single place. For example:

public Foo(int x, int y)
{
    this.x = x;
    this.y = y;
    precomputedValue = x * y;
}

private static int DefaultY
{
    get { return DateTime.Now.Minute; }
}

public Foo(int x) : this(x, DefaultY)
{
}

public Foo() : this(1, DefaultY)
{
}

Note that if you have lots of constructor overloads, you may wish to move to static factory methods instead - that usually makes the code clearer, as well as allowing multiple methods to take the same set of parameters, e.g.

public static XmlDocument FromText(string xml)

public static XmlDocument FromFile(string filename)
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Unfortunately, this technique does not work for runtime default values. In the example, you show constants for the optional parameters. If those values were not constants, then you could not compile the code - would would have to switch to providing the values in the constructor bodies. – JeremyDWill Nov 12 '08 at 19:32
  • Nope, they can be runtime values fetched by executing static methods/properties. Edited answer to give an example. – Jon Skeet Nov 12 '08 at 19:36
  • You still moved the values to a method body, just not the constructor body. That really does not change the situation. – JeremyDWill Nov 12 '08 at 20:03
  • @Jeremy: I don't see your object, I'm afraid. The value of the "default" for y is clearly evaluated at execution time - it relies on the current date. It's not a constant. Please clarify what you believe can't be done. – Jon Skeet Nov 12 '08 at 20:15
  • @Jon: not enough space here for a full example. My basic point is that you can't inline runtime default values - you must create additional methods, which then matches the OPs example, just with a re-ordering of the call chain. The constructor calls only accept static or const value sources. – JeremyDWill Nov 12 '08 at 20:30
  • You have to be able to evaluate the value in a single expression, yes - but at that point it can be inlined. (For instance, I could have put "DateTime.Now.Minute" in the "this" call directly.) I don't really see how that relates to the OP's example though, which has the assignments scattered around. – Jon Skeet Nov 12 '08 at 20:42
  • You're scattering the evaluations instead of the assignments - trading one decentralization for another. To obtain runtime default values, you have to create separate methods, unless you're lucky enough to already have a static method created for that value. I just didn't see how that helped. – JeremyDWill Nov 12 '08 at 21:05
  • 1
    The difference being that the evaluations are named - so it's still obvious what's going on, if you name things appropriately. You don't need to look at each constructor in detail. In my experience this is rarely required anyway, to be honest - either constants or single-expressions are more common. – Jon Skeet Nov 12 '08 at 21:29
  • 1
    To each their own, I guess. In my experience, the default values come from files, databases, or the results of calculations. So creating extra static methods just to serve this construction model would be a poorer practice than performing the assignments in the constructors. Thanks for the dialog! – JeremyDWill Nov 12 '08 at 21:54
-7

2016 Edit: Still ahead of the time, C# is radically cutting back or eliminating its records support and default constructor support for C# 7, maybe C# 8 finally.

2015 Edit: I was far ahead of the time. C#6 and C#7 are removing the need for constructors.

If you're developing in .Net 3.5 I recommend never using constructors. The only exception I leave for this is if you are using an injection constructor for dependency injection. With .Net 3.5 they created object initializers which allow you to do

var myclass = New MyClass { arg1 = "lala", arg2 ="foo" }

This will initalize the class with values assigned for arg1 and arg2 while leaving arg3 as 

default(typeof(arg3)).
Chris Marisic
  • 32,487
  • 24
  • 164
  • 258
  • 8
    And bang goes any opportunity to use readonly variables... – Jon Skeet Nov 12 '08 at 20:16
  • 2
    I don't regard strings and integers dependencies - but they're very often readonly members. For instance your "MyClass" example could certainly have desired arg1 and arg2 to be readonly. Fortunately with optional and named parameters, C# 4 will make this slightly easier. – Jon Skeet Nov 12 '08 at 20:43
  • 2
    So... what you are saying is to use constructors with named values, basically? Oh, and I really like exposing all my private internals just so I can get an unglier initialization syntax. So great, if I need named params I'll use 'em. – Andrew Nov 12 '08 at 22:35
  • I resoundingly stand by my answer 4 years later. – Chris Marisic Sep 10 '12 at 20:37
  • But what's the benefit of your first example over constructors? – Chris Laplante Sep 13 '12 at 19:19
  • @SimpleCoder with ctors you either will need to create a ctor for every single combination of the properties you have or everytime you add a property you will need to update every single usage of the ctor. Talk about onus on the developer. – Chris Marisic Sep 13 '12 at 19:45
  • That's true for C# 3.5 and earlier, but with 4.0 and later, you can use named parameters and optional parameters in constructors. – Chris Laplante Sep 13 '12 at 19:48
  • @SimpleCoder you want a constructor that has 45 optional arguments? – Chris Marisic Sep 13 '12 at 20:01
  • 1
    Not everything has to be set in the constructor/object initializer! Constructors are there to provide convenient shortcuts to object initialization, not a way to set every property *ever* in one go. Your argument makes it seem that you are opposed to manually setting properties. Constructors are also the only way to configure internal state upon initialization (especially if it's dependent on the constructor signature used), unless, as Andrew alludes to, you don't use private/internal fields. – Chris Laplante Sep 13 '12 at 21:25
  • @SimpleCoder Pretty much the only purpose of fields are for constructor injected dependencies that are private readonly. – Chris Marisic Sep 17 '12 at 18:26
  • 4
    That's incorrect. Backing fields are used with properties all the time. – Chris Laplante Sep 17 '12 at 20:38
  • 1
    8 years later still waiting for c# to fully catch up to me. – Chris Marisic Feb 27 '20 at 17:02