Dependency Inversion Principle (DIP) Comments

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:

  • BidsForSniperMessageListener.handleBiddingStateEvent(Chat, BiddingState)
  • UpdatesMainWindowMessageListener.handleBiddingStateEvent(BiddingState)

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.

Now that 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 chat parameter.
  • Both subclasses need a reference back to Main, but the superclass FooMessageListener doesn’t need it. Of course, that says more about Main in general than the FooMessageListener hierarchy: Main violates 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:

  1. Make FooMessageListener an event source for this new type of event, which I’ll tentatively call an AuctionEvent.
  2. Turn the subclasses into AuctionEventListeners.
  3. Inline stuff back into place.

You can follow the detailed changes on this branch starting with this commit.

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 SMELL and 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 MessageListeners to AuctionEventListeners.

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.

Comments

Design credit: Shashank Mehta