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);
}
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);
}
}
The expected usage for the constructor then becomes:
new DispatcherUsingConfiguration(config)
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);
}
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));
}
}
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
}
}
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;
}
}
And the usage becomes:
new DispatcherWithoutConfiguration(new SenderFactory().build(config))
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.
