Constructors that do too much

A flaw I've notice frequently in a legacy code base is constructors which are doing too much work. A common anti-pattern is that a configuration is passed into a constructor. This configuration is then perused to find information so that further objects can be constructed in the constructor. I will discuss why I think this is bad practice.

As an example I have a message dispatcher which delegates to a sender object
to send a message to a destination. A dispatcher usually makes a decision on the appropriate destination based on the message. For this example there is only one endpoint. This destination is to be read from the configuration.

So I have an interface for the sender:

	interface ISender {
	public void send(String message);
	}

view raw sender-interface.java hosted with ❤ by GitHub

and a dispatcher which uses the configuration to build a sender inside its constructor:

	class DispatcherUsingConfiguration {
	private ISender sender;
	public DispatcherUsingConfiguration(Map<String,String> config) {
	sender = new Sender(config);
	}
	public void sendMessage(String message) {
	sender.send(message);
	}
	}

view raw DispatcherUsingConfiguration.java hosted with ❤ by GitHub

The expected usage for the constructor then becomes:

new DispatcherUsingConfiguration(config)

view raw DispatcherUsingConfiguration-ctor.java hosted with ❤ by GitHub

This is bad on several counts:

• Abstract data type is passed in. There's no indication how this will be used or what it's used for. It's just a bucket of parameters. If possible a specific type should be used in preference to an ADT. It's all about the domain and context. Check out Domain Driven Design  or GOOS touches on this point.
• Harder to unit test. I have to make sure configuration is correct so that internal objects can be created properly. In my example I've only one object to consider but they may be several. 
• I can't mock the behaviour of any objects created by the constructor. In essence that behaviour is hard-wired which may be deleterious to testing if that behaviour is using a real resource i.e. socket or database. It turns the unit test into more of an integration test

Use a factory approach

So instead of passing in a configuration, pass in the already constructed objects. A DI framework such as Spring or Guice will assist you in this regard as it's fundamentally what DI is all about. You inject pre-configured objects into other objects rather than building those objects internally.

If you're not using a DI framework (I'm in this situation as it's legacy code) then it's better to pass a factory which builds the object on the fly as a constructor parameter.

To illustrate this:

I can have a factory that produces Sender objects. I've taken the liberty of introducing a generic factory for this purpose. It builds objects of the specified type for a given configuration.

	public interface IBuildFromConfigFactory<M> {
	public M build(Map<String,String> config);
	}

view raw IBuildFromConfigFactory.java hosted with ❤ by GitHub

Now I provide an implementation for the Sender:

	static class SenderFactory implements IBuildFromConfigFactory<ISender> {
	private static final String DESTINATION = "send-to";
	@Override
	public ISender build(Map<String, String> config) {
	return new Sender(config.get(DESTINATION));
	}
	}

view raw SenderFactory.java hosted with ❤ by GitHub

And to illustrate the point I rewrite the Sender class:

	static class Sender implements ISender {
	private final String destination;
	public Sender (String destination) {
	this.destination = destination;
	}
	@Override
	public void send(String message) { //TODO: Send message to destination
	}
	}

view raw Sender.java hosted with ❤ by GitHub

Finally I rewrite the message dispatcher. The constructor now takes objects rather than configuration

	static class DispatcherWithoutConfiguration {
	private final ISender sender;
	public DispatcherWithoutConfiguration(ISender sender) {
	this.sender = sender;
	}
	}

view raw DispatcherWithoutConfiguration.java hosted with ❤ by GitHub

And the usage becomes:

new DispatcherWithoutConfiguration(new SenderFactory().build(config))

view raw DispatcherWithoutConfiguration.java hosted with ❤ by GitHub

This is a lot better:

• Now the dispatcher only knows about objects. It does not even know a configuration is used.
• The transformation of a configuration into object is localized in the factory and not spread out across the code base
• The class becomes trivial to unit test. Instead of worrying about configurations I can simply create a mock Sender and pass that to the dispatcher. In this way I can focus solely on the behaviour of the Dispatcher itself.

Conclusion

Resist the urge to pass around configuration objects and allowing constructors to build new objects. A constructor should ideally be used for assembly only. It's better to just pass pre-configured objects into a constructor. This makes the intention of that constructor clear and makes it easier to unit test the class in question as the colloborators can be mocked easily.