Flaw: Constructor does Real Work

Question

I have a class which represents a set of numbers. The constructor takes three arguments: startValue, endValue and stepSize. The class is responsible for holding a list containing all values between start and end value taking the stepSize into consideration.

Example: startValue: 3, endValue: 1, stepSize = -1, Collection = { 3,2,1 }

I am currently creating the collection and some info strings about the object in the constructor. The public members are read only info strings and the collection.

My constructor does three things at the moment:

• Checks the arguments; this could throw an exception from the constructor

• Fills values into the collection

• Generates the information strings

I can see that my constructor does real work but how can I fix this, or, should I fix this? If I move the "methods" out of the constructor it is like having init function and leaving me with an not fully initialized object. Is the existence of my object doubtful? Or is it not that bad to have some work done in the constructor because it is still possible to test the constructor because no object references are created.

For me it looks wrong but it seems that I just can't find a solution. I also have taken a builder into account but I am not sure if that's right because you can't choose between different types of creations. However single unit tests would have less responsibility.

I am writing my code in C# but I would prefer a general solution, that's why the text contains no code.

EDIT: Thanks for editing my poor text (: I changed the title back because it represents my opinion and the edited title did not. I am not asking if real work is a flaw or not. For me, it is. Take a look at this reference.

http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

The blog states the problems quite well. Still I can't find a solution.

Answer 1

Concepts that urge you to keep your constructors light weight:

• Inversion of control (Dependency Injection)
• Single responsibility principle (as applied to the constructor rather than a class)
• Lazy initialization
• Testing
• K.I.S.S.
• D.R.Y.

Links to arguments of why:

• How much work should be done in a constructor?
• What (not) to do in a constructor
• Should a C++ constructor do real work?
• http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

If you check the arguments in the constructor that validation code can't be shared if those arguments come in from any other source (setter, constructor, parameter object)

If you fill values into the collection or generate the information strings in the constructor that code can't be shared with other constructors you may need to add later.

In addition to not being able to be shared there is also being delayed until really needed (lazy init). There is also overriding thru inheritance that offers more options with many methods that just do one thing rather then one do everything constructor.

Your constructor only needs to put your class into a usable state. It does NOT have to be fully initialized. But it is perfectly free to use other methods to do the real work. That just doesn't take advantage of the "lazy init" idea. Sometimes you need it, sometimes you don't.

Just keep in mind anything that the constructor does or calls is being shoved down the users / testers throat.

EDIT:

You still haven't accepted an answer and I've had some sleep so I'll take a stab at a design. A good design is flexible so I'm going to assume it's OK that I'm not sure what the information strings are, or whether our object is required to represent a set of numbers by being a collection (and so provides iterators, size(), add(), remove(), etc) or is merely backed by a collection and provides some narrow specialized access to those numbers (such as being immutable).

This little guy is the Parameter Object pattern

/** Throws exception if sign of endValue - startValue != stepSize */
ListDefinition(T startValue, T endValue, T stepSize);

T can be int or long or short or char. Have fun but be consistent.

/** An interface, independent from any one collection implementation */
ListFactory(ListDefinition ld){
    /** Make as many as you like */
    List<T> build();
}

If we don't need to narrow access to the collection, we're done. If we do, wrap it in a facade before exposing it.

/** Provides read access only.  Immutable if List l kept private. */
ImmutableFacade(List l);

Oh wait, requirements change, forgot about 'information strings'. :)

/** Build list of info strings */
InformationStrings(String infoFilePath) {
     List<String> read();
}

Have no idea if this is what you had in mind but if you want the power to count line numbers by twos you now have it. :)

/** Assuming information strings have a 1 to 1 relationship with our numbers */
MapFactory(List l, List infoStrings){
    /** Make as many as you like */
    Map<T, String> build();
}

So, yes I'd use the builder pattern to wire all that together. Or you could try to use one object to do all that. Up to you. But I think you'll find few of these constructors doing much of anything.

EDIT2
I know this answer's already been accepted but I've realized there's room for improvement and I can't resist. The ListDefinition above works by exposing it's contents with getters, ick. There is a "Tell, don't ask" design principle that is being violated here for no good reason.

ListDefinition(T startValue, T endValue, T stepSize) {
    List<T> buildList(List<T> l);
}

This let's us build any kind of list implementation and have it initialized according to the definition. Now we don't need ListFactory. buildList is something I call a shunt. It returns the same reference it accepted after having done something with it. It simply allows you to skip giving the new ArrayList a name. Making a list now looks like this:

ListDefinition<int> ld = new ListDefinition<int>(3, 1, -1);
List<int> l = new ImmutableFacade<int>(  ld.buildList( new ArrayList<int>() )  );

Which works fine. Bit hard to read. So why not add a static factory method:

List<int> l = ImmutableRangeOfNumbers.over(3, 1, -1);

This doesn't accept dependency injections but it's built on classes that do. It's effectively a dependency injection container. This makes it a nice shorthand for popular combinations and configurations of the underlying classes. You don't have to make one for every combination. The point of doing this with many classes is now you can put together whatever combination you need.

Well, that's my 2 cents. I'm gonna find something else to obsess on. Feedback welcome.

Answer 2

As far as cohesion is concerned, there's no "real work", only work that's in line (or not) with the class/method's responsibility.

A constructor's responsibility is to create an instance of a class. And a valid instance for that matter. I'm a big fan of keeping the validation part as intrinsic as possible, so that you can see the invariants every time you look at the class. In other words, that the class "contains its own definition".

However, there are cases when an object is a complex assemblage of multiple other objects, with conditional logic, non-trivial validation or other creation sub-tasks involved. This is when I'd delegate the object creation to another class (Factory or Builder pattern) and restrain the accessibility scope of the constructor, but I think twice before doing it.

In your case, I see no conditionals (except argument checking), no composition or inspection of complex objects. The work done by your constructor is cohesive with the class because it essentially only populates its internals. While you may (and should) of course extract atomic, well identified construction steps into private methods inside the same class, I don't see the need for a separate builder class.