We’ve read this before: don’t reuse code by subclassing. Instead, compose objects into a loose network that share responsibility for the system’s behavior. While practising, working through Growing Object-Oriented Systems, I reached the point where Steve and Nat started writing microtests, and decided to stray from the text and try it out on my own. I reached the stage where the first two system tests pass, but in the process, I introduced much cyclic dependency and duplication, particularly related to sending and parsing chat messages. In preparing to address this problem, I noticed some duplication in how the auction handles its incoming chats: sometimes it updated the UI, and sometimes it didn’t. I figured I’d turn those into two chat message handlers, separating the UI behavior from the non-UI behavior, and in so doing, introduced some temporary duplication.
You can probably see a straightforward way to start removing this duplication. To illustrate it, look at the following code snippet.
So we have two classes with a common field, a common method signature, and similar method bodies. Extract Superclass seems logical, even though I can’t think of a decent name for this class so far. No worry: it will come to me.
Next, we have to extract the parts that differ from the parts the match each other. I did that by introducing new methods.
Next, we make
processMessage() identical in both classes, so as to pull it up into
FooMessageListener. And here, we find our first warning sign about this refactoring. The signatures of
handleBiddingStateEvent() don’t match:
In order to find out whether this will become a problem, I have to add
Chat to the parameter list for
UpdatesMainWindowMessageListener’s implementation of the method, at least for now. I imagine I could do something more clever, but I’m not sure that introducing a closure over a
Chat object would simplify matters. I can put that on my “to try” list for now. In the meantime, I add the
Chat parameter. See the diff.
Now to turn similar
processMessage() implementations into identical ones, I introduced an empty method. This is another warning sign, since I can’t envision a “default” UI update. Still, I reserve judgment until I have more evidence, and introduce the necessary changes. See the diff.
processMessage() looks identical in both subclasses, we can pull it up into
FooMessageListener, for which we still don’t have a good name. Either see the diff or look at the final result below.
Now that I look at this last version of
Main, I see the value in having attempted this refactoring. It clarifies quite well the specific reasons why I won’t let inheritance play a long-term role in this part of the design. Specifically, look at the little problems with this design.
I still can’t think of a good name for
FooMessageListener, although maybe you’re screaming one at me as you read. (Sorry; I can’t hear you.)
BidsForSniperMessageListener.handleAllOtherEvents()doesn’t need to do anything, which might or might not be a problem, but often points to the Refused Bequest smell.
UpdatesMainWindowMessageListener.handleBiddingStateEvent()doesn’t need its
Both subclasses need a reference back to
Main, but the superclass
FooMessageListenerdoesn’t need it. Of course, that says more about
Mainin general than the
Mainviolates the Single Responsibility Principle in a variety of delightful ways. That’s why we’re here.
So let me talk this through: we have subclasses of
FooMessageListener that respond to the same logical stimuli, but different subclasses need different data and sometimes a subclasses doesn’t need to respond to the stimuli at all. That sounds an awful lot like an event mechanism to me.
And if you look ahead in Steve and Nat’s book, that’s not surprising.
So how do we get from here to there? I plan to attack it this way:
FooMessageListeneran event source for this new type of event, which I’ll tentatively call an
Turn the subclasses into
- Inline stuff back into place.
By the end of this session, I ended up with a new Auction Event concept, which has started separating the “chat message” concept from the “auction changed” event. It does, however, lead to other questions, which I’ve highlighted with
REFACTOR comments, so look for them. For now, I feel good that, while introducing a hierarchy had its problems, it helped me see how specifically to eliminate it. I trusted the mechanics, and it helped me see where to go next. I know I need to introduce an “auction closed” event and perhaps need to introduce an
AuctionEvent object to eliminate the need for type checking in
AuctionEventSourceMessageListener. We’ll go there next time, but in the meantime, peruse the current state of the code.
If you prefer, look at the changes in moving from
I hope I’ve shown how a code base can reject an attempt to reuse code by inheritance, prefering instead, in this case, composition implemented as an event chain.