Sometimes, during the TDD process, I end up with a constructor that has a long, unwieldy list of arguments. Most likely, I got there by adding the object's peers (other objects it has to communicate with) one at a time, and it got out of hand. This is not dreadful, since the process helped me sort out the design of the object and its neighbours, but now it's time to clean up. The object must still be constructed in a valid state as required by the application. I will still need all the current constructor arguments, so I should see if there's any implicit structure there that I can tease out.
One possibility is that some of the arguments together define a concept that should be packaged up and replaced with a new object that represents it. Here's a small example.
public class MessageProcessor {
public MessageProcessor(MessageUnpacker unpacker,
AuditTrail auditer,
CounterPartyFinder counterpartyFinder,
LocationFinder locationFinder,
DomesticNotifier domesticNotifier,
ImportedNotifier importedNotifier) {
// set the fields here
}
public void onMessage(Message rawMessage) {
UnpackedMessage unpacked = unpacker.unpack(rawMessage, counterpartyFinder);
auditer.recordReceiptOf(unpacked);
// some other activity here
if (locationFinder.isDomestic(unpacked)) {
domesticNotifier.notify(unpacked.asDomesticMessage());
} else {
importedNotifier.notify(unpacked.asImportedMessage())
}
}
}
Just the thought of writing expectations for all these objects makes me wilt, which suggests that things are too complicated. A first step is to notice that the unpacker and counterpartyFinder are always used together, they're fixed at construction and one calls the other. I can remove one argument by pushing the counterpartyFinder into the unpacker.
public class MessageProcessor {
public MessageProcessor(MessageUnpacker unpacker,
AuditTrail auditer,
LocationFinder locationFinder,
DomesticNotifier domesticNotifier,
ImportedNotifier importedNotifier) {
…
public void onMessage(Message rawMessage) {
UnpackedMessage unpacked = unpacker.unpack(rawMessage);
…
Then there's the triple of locationFinder and the notifiers, which seem to go together. It might make sense to package them together into a MessageDispatcher.
public class MessageProcessor {
public MessageProcessor(MessageUnpacker unpacker,
AuditTrail auditer,
MessageDispatcher dispatcher) {
…
public void onMessage(Message rawMessage) {
UnpackedMessage unpacked = unpacker.unpack(rawMessage);
auditer.recordReceiptOf(unpacked);
// some other activity here
dispatcher.dispatch(unpacked);
}
}
Although I've forced this example to get it to fit within a blog post, it shows that being sensitive to complexity in the tests can help me clarify my designs. Now I have a message handling object that clearly performs the usual three stages: receive, process, and forward. I've pulled out the message routing code (the MessageDispatcher), so the MessageProcessor has fewer responsiblities and I know where to put routing decisions when things get more complicated. You might also notice that this code is easier to test.
When I'm extracting implicit components, I look first for two conditions: arguments that are always used together in the class, and that have the same lifetime. That usually finds me the concept, then I have the harder task of finding a good name.
As an aside, one of the signs that the design is developing nicely is that this kind of change is easy to integrate. All I have to do is find where the MessageProcessor is created and change this
messageProceesor =
new MessageProcessor(new XmlMessageUnpacker(), auditer, counterpartyFinder,
locationFinder, domesticNotifier, importedNotifier);
to this
messageProceesor =
new MessageProcessor(
new XmlMessageUnpacker(counterpartyFinder), auditer
new MessageDispatcher(locationFinder, domesticNotifier,
importedNotifier));
Later I can cut down the noise by extracting out the creation of the MessageDispatcher.
6 comments:
This is a good example. I would say that you can also aggregate the concept of unpacking and auditing into a single MessageAcceptor class. Logically calling MessageAcceptor.accept(msg) should do all of the chain logic required to accept a message for processing including audit logging, historical logging, unpacking it, verifying security signatures etc etc. You can use the chain of responsibility pattern here for maximum flexibility. Also note that this focuses the business logic to: (1) accept message for processing (2) process it (3) log processing success ... which is as it should be.
Agreed. I stopped there because I didn't want to overdo the refactoring within a single posting.
This example also shows the problem with using interaction based tests. If this code was written using TDD then you probably already have some tests which are checking the old set of interactions. Extracting these new concepts requires you to change all of these existing tests and defining a new set of interaction tests instead, even though you didn't change the semantics of the operation.
There's a trade-off between writing tests that never have to change, whatever you do to the code, and tests that tell you something useful about the code.
Don't forget that the consequences of writing higher-level tests are less direct error reporting and a combinatorial explosion as you exercise more moving parts at at a time, plus you have to reach into the object model to make assertions about the state of the cluster of objects.
We find there's a difference in style between interaction- and state- TDD, which someone posted on the list a while ago. With interaction-testing, the design process tends to be more continuous since we're constantly thinking about how objects interact. With state-testing we find we tend to code for a while and then clean up under the protection of the tests, so the tests have less of a direct influence on the design.
Totally agree with all of you, guys.
This is a very good example. I even enjoying the reading. Thank you for that.
I have one question about this: although you cut down on the number of arguments the constructor has, unless I'm reading this wrong, you still haven't eliminated the number of objects in play when trying to construct this object.
Generally at the top layer of a codebase there is a (for lack of a better word) controller of sorts that is in charge of instantiating and constructing a large number of objects if your app is correctly isolating components for testability. Is this somewhat of a necessary evil or are there alternatives?
I can think of some other options such as using a virtual factory method to generate your dependency which you can override to return a mock object in tests but that sometimes has its own side effects (calling virtual methods in constructor, having to manually create a fake object because it is at the top layer and you don't want to mock the object whose code you are trying to test).
I'd be interested in hearing your thoughts. Thanks!
Post a Comment