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.

package ca.jbrains.auction.test;

public class Main implements MessageListener {
    // ...
    private void joinAuction(final XMPPConnection connection, String itemId)
            throws XMPPException {

        disconnectWhenUiCloses(connection);
        final Chat chat = connection.getChatManager().createChat(
                auctionId(itemId, connection), new MessageListener() {
                    // SMELL This nested class makes cyclic dependencies too
                    // easy
                    @Override
                    public void processMessage(Chat chat, Message message) {
                        // REFACTOR Replace conditional with polymorphism in
                        // each

                        // SMELL This duplicates code in Main's message
                        // listener,
                        // which probably means an abstraction in the middle is
                        // missing.
                        // REFACTOR? MessageListener parses messages and fires
                        // auction events; AuctionEventListener updates UI or
                        // sends chat message
                        final Object event = Messages.parse(message);
                        if (event instanceof BiddingState) {
                            BiddingState biddingState = (BiddingState) event;
                            if (!Main.SNIPER_XMPP_ID.equals(biddingState
                                    .getBidderName())) {
                                counterBid(chat);
                            }
                        }
                    }
                });

        chat.addMessageListener(this);

        this.dontGcMeBro = chat;

        chat.sendMessage(Messages.joinAuction());
    }
    
    //...
    
    @Override
    public void processMessage(Chat chat, Message message) {
        // SMELL This duplicates code in joinAuction()'s message listener,
        // which probably means an abstraction in the middle is
        // missing.
        // REFACTOR? MessageListener parses messages and fires
        // auction events; AuctionEventListener updates UI or
        // sends chat message
        Object event = Messages.parse(message);
        if (event instanceof BiddingState) {
            BiddingState biddingState = (BiddingState) event;
            if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                signalSniperIsBidding();
            }
        } else {
            signalAuctionClosed();
        }
    }
}

You can probably see a straightforward way to start removing this duplication. To illustrate it, look at the following code snippet.

package ca.jbrains.auction.test;

public class Main {
    public static final class BidsForSniperMessageListener implements
            MessageListener {

        private Main main;

        public BidsForSniperMessageListener(Main main) {
            this.main = main;
        }

        // SMELL This nested class makes cyclic dependencies too
        // easy
        @Override
        public void processMessage(Chat chat, Message message) {
            // REFACTOR Replace conditional with polymorphism in
            // each

            // SMELL This duplicates code in Main's message
            // listener,
            // which probably means an abstraction in the middle is
            // missing.
            // REFACTOR? MessageListener parses messages and fires
            // auction events; AuctionEventListener updates UI or
            // sends chat message
            final Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    main.counterBid(chat);
                }
            }
        }
    }

    public static class UpdatesMainWindowMessageListener implements
            MessageListener {
        private final Main main;

        public UpdatesMainWindowMessageListener(Main main) {
            this.main = main;
        }

        @Override
        public void processMessage(Chat chat, Message message) {
            // SMELL This duplicates code in joinAuction()'s message listener,
            // which probably means an abstraction in the middle is
            // missing.
            // REFACTOR? MessageListener parses messages and fires
            // auction events; AuctionEventListener updates UI or
            // sends chat message
            Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    main.signalSniperIsBidding();
                }
            } else {
                main.signalAuctionClosed();
            }
        }
    }

    //...
}

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.

package ca.jbrains.auction.test;

public class Main {
    public static class FooMessageListener implements MessageListener {
        @Override
        public void processMessage(Chat chat, Message message) {
        }
    }

    public static final class BidsForSniperMessageListener extends
            FooMessageListener {
        
        private final Main main;

        public BidsForSniperMessageListener(Main main) {
            this.main = main;
        }

        @Override
        public void processMessage(Chat chat, Message message) {
            final Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    main.counterBid(chat);
                }
            }
        }
    }

    public static class UpdatesMainWindowMessageListener extends
            FooMessageListener {

        private final Main main;

        public UpdatesMainWindowMessageListener(Main main) {
            this.main = main;
        }

        @Override
        public void processMessage(Chat chat, Message message) {
            Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    main.signalSniperIsBidding();
                }
            } else {
                main.signalAuctionClosed();
            }
        }
    }
}

Next, we have to extract the parts that differ from the parts the match each other. I did that by introducing new methods.

package ca.jbrains.auction.test;

public class Main {
    public static class FooMessageListener implements MessageListener {
        @Override
        public void processMessage(Chat chat, Message message) {
        }
    }

    public static final class BidsForSniperMessageListener extends
            FooMessageListener {
        
        private final Main main;

        public BidsForSniperMessageListener(Main main) {
            this.main = main;
        }

        // SMELL This nested class makes cyclic dependencies too
        // easy
        @Override
        public void processMessage(Chat chat, Message message) {
            final Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                handleBiddingStateEvent(chat, biddingState);
            }
        }

        private void handleBiddingStateEvent(Chat chat,
                BiddingState biddingState) {
            if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                main.counterBid(chat);
            }
        }
    }

    public static class UpdatesMainWindowMessageListener extends
            FooMessageListener {

        private final Main main;

        public UpdatesMainWindowMessageListener(Main main) {
            this.main = main;
        }

        @Override
        public void processMessage(Chat chat, Message message) {
            Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                handleBiddingStateEvent(biddingState);
            } else {
                handleAllOtherEvents();
            }
        }

        private void handleAllOtherEvents() {
            main.signalAuctionClosed();
        }

        private void handleBiddingStateEvent(BiddingState biddingState) {
            if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                main.signalSniperIsBidding();
            }
        }
    }
    //...
}

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.

package ca.jbrains.auction.test;

public class Main {
    public static abstract class FooMessageListener implements MessageListener {
        @Override
        public void processMessage(Chat chat, Message message) {
            final Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                handleBiddingStateEvent(chat, biddingState);
            } else {
                handleAllOtherEvents();
            }
        }

        protected abstract void handleAllOtherEvents();

        protected abstract void handleBiddingStateEvent(Chat chat,
                BiddingState biddingState);
    }

    public static final class BidsForSniperMessageListener extends
            FooMessageListener {

        private final Main main;

        public BidsForSniperMessageListener(Main main) {
            this.main = main;
        }

        @Override
        protected void handleAllOtherEvents() {
            // I don't need to do anything here
        }

        @Override
        protected void handleBiddingStateEvent(Chat chat,
                BiddingState biddingState) {
            if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                main.counterBid(chat);
            }
        }
    }

    public static class UpdatesMainWindowMessageListener extends
            FooMessageListener {

        private final Main main;

        public UpdatesMainWindowMessageListener(Main main) {
            this.main = main;
        }

        @Override
        protected void handleAllOtherEvents() {
            main.signalAuctionClosed();
        }

        @Override
        protected void handleBiddingStateEvent(
                @SuppressWarnings("unused") Chat chat, BiddingState biddingState) {
            if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                main.signalSniperIsBidding();
            }
        }
    }
    //...
}

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.

package ca.jbrains.auction.test;

public class Main {
    public interface AuctionEventListener {
        void handleNewBiddingState(BiddingState biddingState);

        void handleGenericEvent(Object object);
    }

    public static class AuctionEventSourceMessageListener implements
            MessageListener {

        private final List<AuctionEventListener> listeners;

        public AuctionEventSourceMessageListener() {
            this.listeners = new ArrayList<AuctionEventListener>();
        }

        public void addListener(AuctionEventListener listener) {
            listeners.add(listener);
        }

        @Override
        public void processMessage(Chat chat, Message message) {
            // SMELL Duplicated loops
            final Object event = Messages.parse(message);
            if (event instanceof BiddingState) {
                BiddingState biddingState = (BiddingState) event;
                for (AuctionEventListener each : listeners) {
                    each.handleNewBiddingState(biddingState);
                }
            } else {
                for (AuctionEventListener each : listeners) {
                    each.handleGenericEvent(event);
                }
            }
        }
    }
    //...
    private void joinAuction(final XMPPConnection connection, String itemId)
            throws XMPPException {

        disconnectWhenUiCloses(connection);

        final AuctionEventSourceMessageListener auctionEventSource = new AuctionEventSourceMessageListener();

        final Chat chat = connection.getChatManager().createChat(
                auctionId(itemId, connection), auctionEventSource);

        // SMELL? Programming by accident that I can't add these listeners in
        // the constructor of the Auction Event Source?
        auctionEventSource.addListener(new AuctionEventListener() {
            @Override
            public void handleNewBiddingState(BiddingState biddingState) {
                // REFACTOR? Should "sniper now losing" be an event?
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    counterBid(chat);
                }
            }

            @Override
            public void handleGenericEvent(Object object) {
                // I can ignore this
            }
        });

        auctionEventSource.addListener(new AuctionEventListener() {
            @Override
            public void handleNewBiddingState(BiddingState biddingState) {
                // REFACTOR? Should "sniper now losing" be an event?
                if (!Main.SNIPER_XMPP_ID.equals(biddingState.getBidderName())) {
                    signalSniperIsBidding();
                }
            }

            @Override
            public void handleGenericEvent(Object object) {
                // REFACTOR Introduce an "auction closed" event
                signalAuctionClosed();
            }
        });

        this.dontGcMeBro = chat;

        chat.sendMessage(Messages.joinAuction());
    }
    //...
}

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.