DaedTech

Beware the Bloated Constructor

What’s a Bloated Constructor?

Yesterday, I was going through the version history of a file in some code base (from earliest to most recent) and I saw the following:

public ActiveProduct(Product product, StringVersion driver, Side es)
{
    Side = es;

    try
    {
        if (product == null)
            throw new Exception("Can't create an active product from a null product");

        Logger.LogTimeStudyStart("ActiveProduct create");

        if (driver.IsEmpty())
            CurrentDriver = product.Driver;
        else
            CurrentDriver = driver;

        _Device = Session.Instance.DeviceManager.CreateHI(es, CurrentDriver.ToString());
        _Device.ConnectionStatus += Device_ConnectionStatus;
        _Device.BatteryStatus += Device_BatteryStatus;
    }
    catch (Exception ex)
    {
        ExceptionHandler.HandleException(ex);
        throw ex;
    }
    finally
    {
        Logger.LogTimeStudyEnd("ActiveProduct create", "ActiveProduct: Creating " + driver);
    }

    State = ActiveProductState.Simulated;
    Environments = new SortedList();
    this._Product = product;
    _Options = product.AvailableOptions;
    AvailablePrograms = new ProgramSlots(this) { HIStartIndex = 0 };
    AccessoryPrograms = new ProgramSlots(this) { CanRemoveOverride = true, StartIndex = 4, IsAutoShifting = false };
    VirtualPrograms = new ProgramSlots(this) { IsVirtual = true, CanRemoveOverride = true, SlotConfig = Limit.None };
    SlotCalculator = new ProgramCalculator(this, false);
    VirtualCalculator = new ProgramCalculator(this, true);
    DFS = new DFSCalibration(this);
    Accessories = new Accessories();
    PowerBands = PowerBands.Unknown;
}

This made me sad and tired but no worries, I figured, there were a lot more revisions I hadn’t yet seen. Surely somebody did something about this later. And then, happiness:

public ActiveProduct(Product product, StringVersion driver, Side es)
{
    Initialize(product, driver, es);
}

Sweet! That’s much easier on the eyes. Let’s see just see what Initialize does and call it a day:

private void Initialize(Product product, StringVersion driver, Side es)
{
    Side = es;

    try
    {
        if (product == null)
            throw new Exception("Can't create an active product from a null product");

        Logger.LogTimeStudyStart("ActiveProduct create");

        if (driver.IsEmpty())
            CurrentDriver = product.Driver;
        else
            CurrentDriver = driver;

        _Device = Session.Instance.DeviceManager.CreateDevice(es, CurrentDriver.ToString());
        _Device.ConnectionStatus += Device_ConnectionStatus;
        _Device.BatteryStatus += Device_BatteryStatus;
    }
    catch (Exception ex)
    {
        ExceptionHandler.HandleException(ex);
        throw ex;
    }
    finally
    {
        Logger.LogTimeStudyEnd("ActiveProduct create", "ActiveProduct: Creating " + driver);
    }

    State = ActiveProductState.Simulated;
    Environments = new SortedList();
    this._Product = product;
    _Options = product.AvailableOptions;
    AvailablePrograms = new ProgramSlots(this) { DeviceStartIndex = 0 };
    AccessoryPrograms = new ProgramSlots(this) { CanRemoveOverride = true, HIStartIndex = 4, IsAutoShifting = false };
    VirtualPrograms = new ProgramSlots(this) { IsVirtual = true, CanRemoveOverride = true, SlotConfig = Limit.None };
    SlotCalculator = new ProgramCalculator(this, false);
    VirtualCalculator = new ProgramCalculator(this, true);
    DFS = new DFSCalibration(this);
    Accessories = new Accessories();
    PowerBands = PowerBands.Unknown;
}

Oh, the humanity… looks like someone sprayed a bit of cologne here and nothing more.

So, what’s the problem?

I contend that this is a fundamental design failure. Forget throwing an exception from within a try block as some kind of goto. Forget all of the static stuff like the logging and exception handling and general global state. Forget the singletons. Forget all of the other ancillary problems. The design failure is how complicated (convoluted) this constructor is. I see this enough that I think some people would call it a development pattern (I don’t think any would claim it rises to the level of “Design Pattern”), but I firmly believe that it is an anti-pattern.

This style of code is the product of procedural, command and control thinking that I blogged about some time back. It makes no distinction between creation/initialization of an object and operations of the object. These distinct activities are blurred as the bloated constructor generates logger messages, talks to singletons, instantiates other objects, etc.

From the perspective of the procedural programmer, the constructor should take the role of making sure the object wants for nothing, having all of its creature comforts and having its every desire and whim satisfied. In the object oriented style, where dependencies tend to be inverted, the constructor has a different and more Spartan role. Its only job is to make sure that object initializes into a state where it satisfies its basic invariants (in other words, it makes sure that the object instance starts off in a valid state and nothing more).

Let’s consider why the latter, object-oriented approach tends to be a better fit in object oriented languages (and, I would argue, in any state-based paradigm, but I’ll focus on OOP here).

1. Violating Developer Expectations

Imagine that you’re newly assigned to a code base and you want to start understanding how it works. An excellent way to do this is to write a few unit tests (perhaps to keep or perhaps just for throw-away experimentation). You pick some class that you’d like to test and figure you’ll experiment with its API before digging into its code. So, you create a new instance of that class and…

Oops. Your continuous testing tool crashes. Huh. Guess you’ll have to look in the code now (this is never a promising sign — if you have to inspect a class’s code to understand why it’s crashing, it’s probably not a good abstraction, especially if this is true of the constructor.) So, you look in the code and see that there’s a reference to a lazy-loading singleton that lazy-loads a few other singletons and one of those singletons is trying to read files off of a network somewhere and….

Alright, forget it. Let’s move onto another class. You instantiate that class and your testing tool doesn’t crash, but it does turn red. Huh. Back into the code. Ah, this time the class under test doesn’t touch any static stuff or singletons, but it instantiates another class that does. The exception handling is a little better here, so you just turn the testing tool red rather than completely crashing it, but this class is hosed for testing as well.

As a new developer, you’re instantiating classes and expecting that they’ll be instantiated. What’s happening instead is weird, bad, unpredictable things that force you to open the source code to figure out what on Earth is going on. You’re badly betraying a trust other developers put in you to do what you advertise (i.e. putting together the minimum invariants of the class).

2. Testability Nightmare

And, looking back at the narrative from the previous section, another problem makes itself pretty obvious as well. Constructors that do real work instantiating other things (especially global state) make classes impossible to test in isolation. Even if you don’t use global state and instead have classes all instantiate their collaborators, it’s still tough to test things in isolation.

Imagine that your code works like this: main tells a car to build itself, the car tells its engine and chassis to build themselves, the engine tells the alternator and transmission to build themselves, etc. Now, let’s say that you want to write a test of the car to see what happens when its engine starts out invalid. Just how will that be accomplished? You have no control over its engine — it controls that in its constructor. Even if it exposes engine via an accessor, too little, too late.

3. Casts Dependencies And Assumptions in Stone

If the constructor is busy, it’s likely that it’s talking to global state/and or instantiating things. If it takes arguments, it may be mutating those or paring them or coupling itself to them. But, whatever it’s doing, you have no control over it as a client. So, if you want this object at all, you’re getting it with whatever the constructor decides to foist on you.

Contrast this with any non-constructor method. Any non-constructor method you have the option to call or not, so you have the control. If you want a Car that isn’t started, you can simply not call Car.Start(). However, if the author of Car decides to start it in the constructor, you’re hosed. Not having a started car is not an option for you, buddy. Likewise, if Car instantiates a new SixCylinder() engine and you want a more fuel economical four cylinder, you’re SOL.

4. Performance Problems

Let’s say that I have some class, Critical, that I can’t live without in the application. I need to instantiate a Critical and access some property on it in response to a user request. Let’s say that Critical has a busy constructor that chugs and hums and does all manner of things, taking 5 or 10 seconds at times. Not all of this stuff has anything to do with the property that I want to access in response to the user request.

What are my choices here? Well, I don’t have any. I can like it or lump it. I need new Critical() and the user therefore needs to wait 5 or 10 seconds even though his request should take that number of microseconds. Sure, I can cache my Critical for next time or I can implement a flyweight, but that first time, I’m still needlessly forcing a wait on the user.

So, What Should a Constructor Look Like?

Given all of these arguments against bloated constructors, it should be fairly obvious what I think a constructor ought to look like. It ought to contain very little code. If the class has no dependencies, then it ought to contain no code (or not exist). After all, what are you going to do in the constructor that you can’t do in a field initializer? You’re not reaching into global state somewhere, are you?! Tsk tsk.

If a constructor has parameters, it ought to do nothing but set local fields according to those parameters, and perhaps perform some sort of validation on the parameters to ensure that its invariants can be satisfied with these values. That’s really it. The constructor’s only responsibility is to put the object into a valid state as far as its invariants are concerned, while doing the minimum amount of configuration possible. After all, you want objects where clients can explicitly tell them what to do. Even simple things like hooking to events should be triggered by a client method and not the constructor, ideally. It may seem a bit more cumbersome, but it’s also expressive and clear. Constructors hide side effects where well-named methods promote them from side effects to deliberate effects.

So What About that Constructor We Looked At?

How about this:

public ActiveProduct(Product product, StringVersion driver, Side es)
{
    if (product == null)
        throw new Exception("Can't create an active product from a null product");

    Side = es;
    CurrentDriver = driver;
    this._Product = product;
}

Wow, isn’t that like a ray of sunshine on an otherwise cloudy day? No Initialize() method hiding untold horrors. No nested, contradictory exception handling logic. No logging, no singletons, no static state — simple, minimal and easy to reason about. So, what to be done with all of these things that formerly resided in this constructor?

Well, a number of them were simply initializing properties or backing stores. This can be done with class initializers for sensible defaults and should otherwise be left to clients. After all, we’re here to serve them — not force settings on them. The logging makes no sense — do that somewhere else. If you’re writing a class and you want to know how long the constructor takes (which is a huge warning flag, by the way), write that code where you instantiate it — don’t force this on every conceivable client of your class.

The event wireup can be moved into some kind of “Hookup()” method to allow clients specifically to request this. You could also make it more granular if you like, allowing them to hook up ala carte instead of all at once. As for the singeltons and static state, leave that up to the instantiating client if it wants that done. We might as well leverage the thing that makes static state a design nightmare — the fact that anyone can do anything from anywhere — to move the stink away from us with some “Not in my backyard” attitude. If everyone does this, eventually the global state will be bubbled to some common location where singletons can be slain and static classes converted to instances.

Are You a Lone Crackpot, Erik?

As it turns out, no. Here is what others have to say on the subject:

Microsoft agrees with this take, citing performance:

Do minimal work in the constructor. Constructors should not do much work other than to capture the constructor parameters. The cost of any other processing should be delayed until required.

Misko Hevery agrees, citing problems finding seams for testing:

When your constructor has to instantiate and initialize its collaborators, the result tends to be an inflexible and prematurely coupled design. Such constructors shut off the ability to inject test collaborators when testing.

John Wigger cautions against putting too much code in a constructor:

In dealing with a more complicated OO design, it can be a mistake to put too much initialization logic into constructors. This is especially important for an OO design that uses inheritance significantly.

Gilad Bracha even goes so far as to hypothesize that constructors are harmful:

Constructors come with a host of special rules and regulations: they cannot be overridden like instance methods; they need to call another constructor, or a superclass constructor etc. Try defining mixins in the presence of constructors – it’s tricky because the interface for creating instances gets bundled with the interface of the instances themselves.