Flaw: Constructor does Real Work

Work in the constructor such as: creating/initializing collaborators, communicating with other services, and logic to set up its own state removes seams needed for testing, forcing subclasses/mocks to inherit unwanted behavior. Too much work in the constructor prevents instantiation or altering collaborators in the test.

Warning Signs:

new keyword in a constructor or at field declaration

Static method calls in a constructor or at field declaration

Anything more than field assignment in constructors

Object not fully initialized after the constructor finishes (watch out for initialize methods)

Control flow (conditional or looping logic) in a constructor

CL does complex object graph construction inside a constructor rather than using a factory or builder

Adding or using an initialization block

Why this is a Flaw

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.

It violates the Single Responsibility Principle
When collaborator construction is mixed with initialization, it suggests that there is only one way to configure the class, which closes off reuse opportunities that might otherwise be available. Object graph creation is a full fledged responsibility — a different one from why a class exists in the first place. Doing such work in a constructor violates the Single Responsibility Principle.

Testing Directly is Difficult
Testing such constructors is difficult. To instantiate an object, the constructor must execute. And if that constructor does lots of work, you are forced to do that work when creating the object in tests. If collaborators access external resources (e.g. files, network services, or databases), subtle changes in collaborators may need to be reflected in the constructor, but may be missed due to missing test coverage from tests that weren’t written because the constructor is so difficult to test. We end up in a vicious cycle.

Subclassing and Overriding to Test is Still Flawed
Other times a constructor does little work itself, but delegates to a method that is expected to be overridden in a test subclass. This may work around the problem of difficult construction, but using the “subclass to test” trick is something you only should do as a last resort. Additionally, by subclassing, you will fail to test the method that you override. And that method does lots of work (remember – that’s why it was created in the first place), so it probably should be tested.

It Forces Collaborators on You

Sometimes when you test an object, you don’t want to actually create all of its collaborators. For instance, you don’t want a real MySqlRepository object that talks to the MySql service. However, if they are directly created using new MySqlRepositoryServiceThatTalksToOtherServers() inside your System Under Test (SUT), then you will be forced to use that heavyweight object.

It Erases a “Seam”Seams are places you can slice your codebase to remove dependencies and instantiate small, focused objects. When you do new XYZ() in a constructor, you’ll never be able to get a different (subclass) object created. (See Michael Feathers book Working Effectively with Legacy Code for more about seams).

It Still is a Flaw even if you have Multiple Constructors (Some for “Test Only”)
Creating a separate “test only” constructor does not solve the problem. The constructors that do work will still be used by other classes. Even if you can test this object in isolation (creating it with the test specific constructor), you’re going to run into other classes that use the hard-to-test constructor. And when testing those other classes, your hands will be tied.

Bottom LineIt all comes down to how hard or easy it is to construct the class in isolation or with test-double collaborators.

If it’s hard, you’re doing too much work in the constructor!

If it’s easy, pat yourself on the back.

Always think about how hard it will be to test the object while you are writing it. Will it be easy to instantiate it via the constructor you’re writing? (Keep in mind that your test-class will not be the only code where this class will be instantiated.)

So many designs are full of “objects that instantiate other objects or retrieve objects from globally accessible locations. These programming practices, when left unchecked, lead to highly coupled designs that are difficult to test.” [J.B. Rainsberger, JUnit Recipes, Recipe 2.11]

Recognizing the Flaw

Examine for these symptoms:

The new keyword constructs anything you would like to replace with a test-double in a test? (Typically this is anything bigger than a simple value object).

Any static method calls? (Remember: static calls are non-mockable, and non-injectable, so if you see Server.init() or anything of that ilk, warning sirens should go off in your head!)

Any conditional or loop logic? (You will have to successfully navigate the logic every time you instantiate the object. This will result in excessive setup code not only when you test the class directly, but also if you happen to need this class while testing any of the related classes.)

Think about one fundamental question when writing or reviewing code:

How am I going to test this?

“If the answer is not obvious, or it looks like the test would be ugly or hard to write, then take that as a warning signal. Your design probably needs to be modified; change things around until the code is easy to test, and your design will end up being far better for the effort.” [Hunt, Thomas. Pragmatic Unit Testing in Java with JUnit, p 103 (somewhat dated, but a decent and quick read)]

Note: Constructing value objects may be acceptable in many cases (examples: LinkedList; HashMap, User, EmailAddress, CreditCard). Value objects key attributes are: (1) Trivial to construct (2) are state focused (lots of getters/setters low on behavior) (3) do not refer to any service object.

Fixing the Flaw

Do not create collaborators in your constructor, but pass them in

Move the responsibility for object graph construction and initialization into another object. (e.g. extract a builder, factory or Provider, and pass these collaborators to your constructor).

Example: If you depend on a DatabaseService (hopefully that’s an interface), then use Dependency Injection (DI) to pass in to the constructor the exact subclass of DatabaseService object you need.
To repeat: Do not create collaborators in your constructor, but pass them in. (Don’t look for things! Ask for things!)

If there is initialization that needs to happen with the objects that get passed in, you have three options:

Best Approach using Guice: Use a Provider<YourObject> to create and initialize YourObject‘s constructor arguments. Leave the responsibility of object initialization and graph construction to Guice. This will remove the need to initialize the objects on-the-go. Sometimes you will use Builders or Factories in addition to Providers, then pass in the builders and factories to the constructor.

Best Approach using manual Dependency Injection: Use a Builder, or a Factory, for YourObject‘s constructor arguments. Typically there is one factory for a whole graph of objects, see example below. (So you don’t have to worry about having class explosion due to one factory for every class) The responsibility of the factory is to create the object graph and to do no work. (All you should see in the factory is a whole lot of new keywords and passing around of references). The responsibility of the object graph is to do work, and to do no object instantiation (There should be a serious lack of new keywords in application logic classes).

Only as a Last Resort: Have an init(…) method in your class that can be called after construction. Avoid this wherever you can, preferring the use of another object who’s single responsibility is to configure the parameters for this object. (Often that is a Provider if you are using Guice)

// New and Improved is trivially testable, with any
// test-double objects as collaborators.
class HouseTest extends TestCase {
public void testThisIsEasyAndFlexible() {
Kitchen dummyKitchen = new DummyKitchen();
Bedroom dummyBedroom = new DummyBedroom();
House house =
new House(dummyKitchen, dummyBedroom);
// Awesome, I can use test doubles that
// are lighter weight.
// ...
}
}

This example mixes object graph creation with logic. In tests we often want to create a different object graph than in production. Usually it is a smaller graph with some objects replaced with test-doubles. By leaving the new operators inline we will never be able to create a graph of objects for testing. See: “How to think about the new operator”

Flaw: inline object instantiation where fields are declared has the same problems that work in the constructor has.

Flaw: this may be easy to instantiate but if Kitchen represents something expensive such as file/database access it is not very testable since we could never replace the Kitchen or Bedroom with a test-double.

Flaw: Your design is more brittle, because you can never polymorphically replace the behavior of the kitchen or bedroom in the House.

If the Kitchen is a value object such as: Linked List, Map, User, Email Address, etc., then we can create them inline as long as the value objects do not reference service objects. Service objects are the type most likely that need to be replaced with test-doubles, so you never want to lock them in with direct instantiation or instantiation via static method calls.

Problem: Constructor takes a partially initialized object and has to set it up

Object graph creation (creating and configuring the Gardener collaborator for Garden) is a different responsibility than what the Garden should do. When configuration and instantiation is mixed together in the constructor, objects become more brittle and tied to concrete object graph structures. This makes code harder to modify, and (more or less) impossible to test.

Flaw: The Garden needs a Gardener, but it should not be the responsibility of the Garden to configure the gardener.

Flaw: In a unit test for Garden the workday is set specifically in the constructor, thus forcing us to have Joe work a 12 hour workday. Forced dependencies like this can cause tests to run slow. In unit tests, you’ll want to pass in a shorter workday.

Flaw: You can’t change the boots. You will likely want to use a test-double for boots to avoid the problems with loading and using BootsWithMassiveStaticInitBlock. (Static initialization blocks are often dangerous and troublesome, especially if they interact with global state.)

Have two objects when you need to have collaborators initialized. Initialize them, and then pass them fully initialized into the constructor of the class of interest.

In this example we reach into the global state of an application and get a hold of the RPCClient singleton. It turns out we don’t need the singleton, we only want the User. First: we are doing work (against static methods, which have zero seams). Second: this violates the “Law of Demeter”.

Flaw: We cannot easily intercept the call RPCClient.getInstance() to return a mock RPCClient for testing. (Static methods are non-interceptable, and non-mockable).

Flaw: Why do we have to mock out RPCClient for testing if the class under test does not need RPCClient?(AccountView doesn’t persist the rpc instance in a field.) We only need to persist the User.

Flaw: Every test which needs to construct class AccountView will have to deal with the above points. Even if we solve the issues for one test, we don’t want to solve them again in other tests. For example AccountServlet may need AccountView. Hence in AccountServlet we will have to successfully navigate the constructor.

In the improved code only what is directly needed is passed in: the User collaborator. For tests, all you need to create is a (real or test-double) User object. This makes for a more flexible design and enables better testability.

We use Guice to provide for us a User, that comes from the RPCClient. In unit tests we won’t use Guice, but directly create the User and AccountView.

// The test exposes the brittleness of the Car
class CarTest extends TestCase {
public void testNoSeamForFakeEngine() {
// Aggh! I hate using files in unit tests
File file = new File("engine.config");
Car car = new Car(file);
// I want to test with a fake engine
// but I can't since the EngineFactory
// only knows how to make real engines.
}
}

// Now we can see a flexible, injectible design
class CarTest extends TestCase {
public void testShowsWeHaveCleanDesign() {
Engine fakeEngine = new FakeEngine();
Car car = new Car(fakeEngine);
// Now testing is easy, with the car taking
// exactly what it needs.
}
}

Linguistically, it does not make sense to require a Car to get an EngineFactory in order to create its own engine. Cars should be given readymade engines, not figure out how to create them. The car you ride in to work shouldn’t have a reference back to its factory. In the same way, some constructors reach out to third party objects that aren’t directly needed — only something the third party object can create is needed.

Flaw: Passing in a file, when all that is ultimately needed is an Engine.

Flaw: Creating a third party object (EngineFactory) and paying any assorted costs in this non-injectable and non-overridable creation. This makes your code more brittle because you can’t change the factory, you can’t decide to start caching them, and you can’t prevent it from running when a new Car is created.

Flaw: It’s silly for the car to know how to build an EngineFactory, which then knows how to build an engine. (Somehow when these objects are more abstract we tend to not realize we’re making this mistake).

Flaw: Every test which needs to construct class Car will have to deal with the above points. Even if we solve the issues for one test, we don’t want to solve them again in other tests. For example another test for a Garage may need a Car. Hence in Garage test I will have to successfully navigate the Car constructor. And I will be forced to create a new EngineFactory.

Flaw: Every test will need a access a file when the Car constructor is called. This is slow, and prevents test from being true unit tests.

Remove these third party objects, and replace the work in the constructor with simple variable assignment. Assign pre-configured variables into fields in the constructor. Have another object (a factory, builder, or Guice providers) do the actual construction of the constructor’s parameters. Split off of your primary objects the responsibility of object graph construction and you will have a more flexible and maintainable design.

Problem: Directly Reading Flag Values in Constructor

// Best solution (although you also could pass
// in an int of the Socket's port to use)
class PingServer {
Socket socket;
@Inject
PingServer(Socket socket) {
this.socket = socket;
}
}
// This uses the FlagBinder to bind Flags to
// the @Named annotation values. Somewhere in
// a Module's configure method:
new FlagBinder(
binder().bind(FlagsClassX.class));
// And the method provider for the Socket
@Provides
Socket getSocket(@Named("port") int port) {
// The responsibility of this provider is
// to give a fully configured Socket
// which may involve more than just "new"
return new Socket(port);
}

// The test is brittle and tied directly to a
// Flag's static method (global state).
class PingServerTest extends TestCase {
public void testWithDefaultPort() {
PingServer server = new PingServer();
// This looks innocent enough, but really
// it forces you to mutate global state
// (the flag) to run on another port.
}
}

What looks like a simple no argument constructor actually has a lot of dependencies. Once again the API is lying to you, pretending it is easy to create, but actually PingServer is brittle and tied to global state.

Flaw: In your test you will have to rely on global variable FLAG_PORT in order to instantiate the class. This will make your tests flaky as the order of tests matters.

Flaw: Depending on a statically accessed flag value prevents you from running tests in parallel. Because parallel running test could change the flag value at the same time, causing failures.

Flaw: If the socket needed additional configuration (i.e. calling setSoTimeout()), that can’t happen because the object construction happens in the wrong place. Socket is created inside the PingServer, which is backwards. It needs to happen externally, in something whose sole responsibility is object graph construction — i.e. a Guice provider.

PingServer ultimately needs a socket not a port number. By passing in the port number we will have to tests with real sockets/threads. By passing in a socket we can create a mock socket in tests and test the class without any real sockets / threads. Explicitly passing in the port number removes the dependency on global state and greatly simplifies testing. Even better is passing in the socket that is ultimately needed.

// Testing the CurlingTeamMember is difficult.
// In fact you can't use any Jersey other
// than the SuedeJersey or NylonJersey.
class CurlingTeamMemberTest extends TestCase {
public void testImpossibleToChangeJersey() {
// You are forced to use global state.
// ... Set the flag how you want it
CurlingTeamMember russ =
new CurlingTeamMember();
// Tests are locked in to using one
// of the two jerseys above.
}
}

Guice has something called the FlagBinder which lets you–at a very low cost–remove flag references and replace them with injected values. Flags are pervasively used to change runtime parameters, yet we don’t have to directly read the flags for their global state.

Flaw: Directly reading flags is reaching out into global state to get a value. This is undesirable because global state is not isolated: previous tests could set it to a different value, or other threads could mutate it unexpectedly.

Flaw: Directly constructing the differing types of Jersey, depending on a flag’s value. Your tests that instantiate a CurlingTeamMember have no seam to inject a different Jersey collaborator for testing.

Flaw: The responsibility of the CurlingTeamMember is broad: both whatever the core purpose of the class, and now also Jersey configuration. Passing in a preconfigured Jersey object instead is preferred. Another object can have the responsibility of configuring the Jersey.

Use the FlagBinder (is a class you write which knows how to bind command line flags to injectable parameters) to attach all the flags from a class into Guice’s scope of what is injectable.

// Brittle code exposed through the test
class VisualVoicemailTest extends TestCase {
public void testExposesBrittleDesign() {
User dummyUser = new DummyUser();
VisualVoicemail voicemail =
new VisualVoicemail(dummyUser);
voicemail.setCalls(buildListOfTestCalls());
// Technically this can be tested, as long
// as you don't need the Server to have
// read the config file. But testing
// without testing the initialize()
// excludes important behavior.
// Also, the code is brittle and hard to
// later on add new functionalities.
}
}

Moving the “work” into an initialize method is not the solution. You need to decouple your objects into single responsibilities. (Where one single responsibility is to provide a fully-configured object graph).

Flaw: At first glance it may look like Guice is effectively used. For testing the VisualVoicemail object is very easy to construct. However the code is still brittle and tied to several Static initialization calls.

Flaw: The initialize method is a glaring sign that this object has too many responsibilities: whatever a VisualVoicemail needs to do, and initializing its dependencies. Dependency initialization should happen in another class, passing all of the ready-to-be-used objects into the constructor.

Flaw: The Server.readConfigFromFile() method is non interceptable when in a test, if you want to call the initialize method.

Flaw: The Server is non-initializable in a test. If you want to use it, you’re forced to get it from the global singleton state. If two tests run in parallel, or a previous test initializes the Server differently, global state will bite you.

Flaw: Usually, @VisibleForTesting annotation is a smell that the class was not written to be easily tested. And even though it will let you set the list of calls, it is only a hack to get around the root problem in the initialize() method.

Solving this flaw, like so many others, involves removing JVM enforced global state and using Dependency Injection.

// Testing the VideoPlaylistIndex is easy,
// but testing the PlaylistGenerator is not!
class PlaylistGeneratorTest extends TestCase {
public void testBadDesignHasNoSeams() {
PlaylistGenerator generator =
new PlaylistGenerator();
// Doh! Now we're tied to the
// VideoPlaylistIndex with the bulky
// FullLibraryIndex that will make slow
// tests.
}
}

// Easy to test when Dependency Injection
// is used everywhere.
class PlaylistGeneratorTest extends TestCase {
public void testFlexibleDesignWithDI() {
VideoPlaylistIndex fakeIndex =
new InMemoryVideoPlaylistIndex()
PlaylistGenerator generator =
new PlaylistGenerator(fakeIndex);
// Success! The generator does not care
// about the index used during testing
// so a fakeIndex is passed in.
}
}

Multiple constructors, with some only used for testing, is a hint that parts of your code will still be hard to test. On the left, the VideoPlaylistIndex is easy to test (you can pass in a test-double VideoRepository). However, whichever dependant objects which use the no-arg constructor will be hard to test.

Flaw: PlaylistGenerator is hard to test, because it takes advantage of the no-arg constructor for VideoPlaylistIndex, which is hard coded to using the FullLibraryIndex.You wouldn’t really want to test the FullLibraryIndex in a test of the PlaylistGenerator, but you are forced to.

Flaw: Usually, the @VisibleForTesting annotation is a smell that the class was not written to be easily tested. And even though it will let you set the list of calls, it is only a hack to get around the root problem.

Ideally the PlaylistGenerator asks for the VideoPlaylistIndex in its constructor instead of creating its dependency directly. Once PlaylistGenerator asks for its dependencies no one calls the no argument VideoPlaylistIndex constructor and we are free to delete it. We don’t usually need multiple constructors.

Frequently Asked Questions

Q1: Okay, so I think I get it. I’ll only do assignment in the constructor, and then I’ll have an init() method or init(…) to do all the work that used to be in the constructor. Does that sound okay?
A: We discourage this, see the code example above.

Q2: What about multiple constructors? Can I have one simple to construct, and the other with lots of work? I’ll only use the easy one in my tests. I’ll even mark it @VisibleForTesting. Okay?
A: We discourage this, see the code example above.

Q3: Can I create named constructors as additional constructors which may do work? I’ll have a simple assignment to fields only constructor to use for my tests.
A: Someone actually ask this and we’ll elaborate.

Q4: I keep hearing you guys talk about creating “Factories” and these objects whose responsibility is exclusively construction of object graphs. But seriously, that’s too many objects, and too much for such a simple thing.
A: Typically there is one factory for a whole graph of objects, see example. So you don’t have to worry about having class explosion due to one factory per class. The responsibility of the factory is to create the object graph and to do no work (All you should see is a whole lot of new keywords and passing around of references). The responsibility of the object graph is to do work, and to do no object instantiation.

22 responses so far ↓

Some libraries force us to have a no-args constructor in some cases (probably to help instantiation through reflection). Is it ok in these cases to have a init(…) or setXXX(…) methods? If not how would you circumvent this issue?

Rodrigo: I’m no authority on this as I’m learning as well… but I would think the design is questionable if your class is meant to be instantiated by a third party library and yet requires initialization work. But I would like to know with more detail a case where you need this to see if I can come up with something better…

Does any of this apply to a Client class? (I use the term ‘Client’ in the way GoF does.) Following the principle of having the constructor do no real work has solved all kinds of problems in my patterns, but when I go to call the design, I have to use the constructor method in the Client to instantiate objects. In other words, the constructor in the Client does real work.

Bill,
in my opinion you should not do work in the constructor of a client, and the possible cases are two:
- the Client needs a collaborator with the same lifecycle. So pass it as an argument of the constructor of the Client.
- the Clicnt needs to create an object at a certain point of execution, in a method. So pass in a Factory in the Client’s constructor.

For triangle all interior angles total to 180 degrees.
Since I truly believe that objects should be responsible for ensuring validity of its state, checking if sum of user provided angles equals to 180 should be done in constructor – you would end up having some conditional logic in your constructor…

If invariant is violated i would throw IllegalArgumentException…

This approach seems to be very attractive – you will always work with valid triangles, but it kills testability:
To test any of triangle behavior (for example checking if triangle is right-angled) you first need to instantiate it feeding it with correct angle values.
For triangle example it does not seem to be the case, but in more complex scenarios instantiating object to be tested can be quite hard – not because of its hard coded collaborators but because of its invariants.

Hi
Thanks for a link to your post. I have read it 3 times and I am still not convinced.
Hopefully, after reading comments of this post i can see that I am not the only one who disagrees with you.
The thing i do not understand the most is
“Now there is a difference if the API is meant for my internal consumption or is part of an external API”
How can you guarantee that none of your colleagues will call your “internal consumption API” ? Use access level modifiers ?

I can agree with guys who proposed using some stubs/mocks to pass to House constructor.
I would do the same in this case ( using DI would be even better) but in my triangle scenario it is not about collaborators validation
but validating internal consistency of object.

I’d like to add to this flaw another bad pattern that I’ve seen in PHP code. In PHP you can write code in the global scope and not just in function and method bodies. When you use PHPUnit, every time a file that has this kind of global code, it runs when a test includes the file or when the coverage report’s collector includes it. That means that possibly global state gets modified, and even strange errors may occur. Maybe the file just wants to include another file that is not on the include_path when the tests run, because the tests run from a directory and the file assumes that it will be run from another directory. So generally you at first make sure that this kind of code in the global scope will never ever get included (and I’ve seen codebases where it’s really hard to filter them) and at second you should ensure that this kind of code is kept to a minimum, like for entry points of your webapp (index.php and the others) and every other logic is in method (or at least function) bodies.
Basically it’s the same problem that arises when you use static initialization blocks in java (which you already bashed to death somewhere else ).

[...] 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. [...]

[...] Another problem is testing. We can’t, in a unit test, easily replace Child with a mock or a stub. Maybe instantiation of a child is difficult (it takes 9 months for gods sake!) Or we simply want to test the interaction between Limb and Child, which is difficult if we don’t know to which Child this Limb is talking to. (Remember: A constructor shouldn’t do real work!) [...]