In which we slice up our application, shuffling behavior around to isolate the XMPP and user interface code from the sniping logic. We achieve this incrementally, changing one concept at a time without breaking the whole application. We finally put a stake through the heart of notToBeGCd
.
We’ve convinced ourselves that we need to do some surgery on Main
, but what do we want our improved Main
to do?
For programs that are more than trivial, we like to think of our top-level class as a “matchmaker,” finding components and introducing them to each other. Once that job is done it drops into the background and waits for the application to finish. On a larger scale, this what the current generation of application containers do, except that the relationships are often encoded in XML.
In its current form, Main
acts as a matchmaker but it’s also implementing some of the components, which means it has too many responsibilities. One clue is to look at its imports:
We’re importing code from three unrelated packages, plus the auctionsniper
package itself. In fact, we have a package loop in that the top-level and UI packages depend on each other. Java, unlike some other languages, tolerates package loops, but they’re not something we should be pleased with.
We think we should extract some of this behavior from Main
, and the XMPP features look like a good first candidate. The use of the Smack should be an implementation detail that is irrelevant to the rest of the application.
Most of the action happens in the implementation of UserRequestListener.joinAuction()
within Main
. We notice that we’ve interleaved different domain levels, auction sniping and chatting, in this one unit of code. We’d like to split them up. Here it is again:
The object that locks this code into Smack is the chat
; we refer to it several times: to avoid garbage collection, to attach it to the Auction
implementation, and to attach the message listener. If we can gather together the auction- and Sniper-related code, we can move the chat
elsewhere, but that’s tricky while there’s still a dependency loop between the XMPPAuction
, Chat
, and AuctionSniper
.
Looking again, the Sniper actually plugs in to the AuctionMessageTranslator
as an AuctionEventListener
. Perhaps using an Announcer
to bind the two together, rather than a direct link, would give us the flexibility we need. It would also make sense to have the Sniper as a notification, as defined in “Object Peer Stereotypes” (page 52). The result is:
This looks worse, but the interesting bit is the last three lines. If you squint, it looks like everything is described in terms of Auctions and Snipers (there’s still the Swing thread issue, but we did tell you to squint).
From here, we can push everything to do with chat
, its setup, and the use of the Announcer
, into XMPPAuction
, adding management methods to the Auction
interface for its AuctionEventListener
s. We’re just showing the end result here, but we changed the code incrementally so that nothing was broken for more than a few minutes.
Apart from the garbage collection “wart,” this removes any references to Chat
from Main
.
We also write a new integration test for the expanded XMPPAuction
to show that it can create a Chat
and attach a listener. We use some of our existing end-to-end test infrastructure, such as FakeAuctionServer
, and a CountDownLatch
from the Java concurrency libraries to wait for a response.
Looking over the result, we can see that it makes sense for XMPPAuction
to encapsulate a Chat
as now it hides everything to do with communicating between a request listener and an auction service, including translating the messages. We can also see that the AuctionMessageTranslator
is internal to this encapsulation, the Sniper doesn’t need to see it. So, to recognize our new structure, we move XMPPAuction
and AuctionMessageTranslator
into a new auctionsniper.xmpp
package, and the tests into equivalent xmpp
test packages.
The next thing to remove from Main
is direct references to the XMPPConnection
. We can wrap these up in a factory class that will create an instance of an Auction
for a given item, so it will have a method like
Auction auction = <factory>.auctionFor(item id);
We struggle for a while over what to call this new type, since it should have a name that reflects the language of auctions. In the end, we decide that the concept that arranges auctions is an “auction house,” so that’s what we call our new type:
The end result of this refactoring is:
Implementing XMPPAuctionHouse
is straightforward; we transfer there all the code related to connection
, including the generation of the Jabber ID from the auction item ID. Main
is now simpler, with just one import for all the XMPP code, auctionsniper.xmpp.XMPPAuctionHouse
. The new version looks like Figure 17.2.
For consistency, we retrofit XMPPAuctionHouse
to the integration test for XMPPAuction
, instead of creating XMPPAuction
s directly as it does now, and rename the test to XMPPAuctionHouseTest
.
Our final touch is to move the relevant constants from Main
where we’d left them: the message formats to XMPPAuction
and the connection identifier format to XMPPAuctionHouse
. This reassures us that we’re moving in the right direction, since we’re narrowing the scope of where these constants are used.
Finally, we’d like to do something about the direct reference to the SnipersTableModel
and the related SwingThreadSniperListener
—and the awful notToBeGCd
. We think we can get there, but it’ll take a couple of steps.
The first step is to turn the anonymous implementation of UserRequestListener
into a proper class so we can understand its dependencies. We decide to call the new class SniperLauncher
, since it will respond to a request to join an auction by “launching” a Sniper. One nice effect is that we can make notToBeGCd
local to the new class.
With the SniperLauncher
separated out, it becomes even clearer that the Swing features don’t fit here. There’s a clue in that our use of snipers
, the SnipersTableModel
, is clumsy: we tell it about the new Sniper by giving it an initial SniperSnapshot
, and we attach it to both the Sniper and the auction. There’s also some hidden duplication in that we create an initial SniperSnaphot
both here and in the AuctionSniper
constructor.
Stepping back, we ought to simplify this class so that all it does is establish a new AuctionSniper
. It can delegate the process of accepting the new Sniper into the application to a new role which we’ll call a SniperCollector
, implemented in the SnipersTableModel
.
The one behavior that we want to confirm is that we only join the auction after everything else is set up. With the code now isolated, we can jMock a States
to check the ordering.
where sniperForItem()
returns a Matcher
that matches any AuctionSniper
associated with the given item identifier.
We extend SnipersTableModel
to fulfill its new role: now it accepts AuctionSniper
s rather than SniperSnapshot
s. To make this work, we have to convert a Sniper
’s listener from a dependency to a notification, so that we can add a listener after construction. We also change SnipersTableModel
to use the new API and disallow adding SniperSnapshot
s.
One change that suggests that we’re heading in the right direction is that the SwingThreadSniperListener
is now packaged up in the Swing part of the code, not in the generic SniperLauncher
.
As a next step, we realize that we don’t yet have anything that represents all our sniping activity and that we might call our portfolio. At the moment, the SnipersTableModel
is implicitly responsible for both maintaining a record of our sniping and displaying that record. It also pulls a Swing implementation detail into Main
.
We want a clearer separation of concerns, so we extract a SniperPortfolio
to maintain our Snipers, which we make our new implementer of SniperCollector
. We push the creation of the SnipersTableModel
into MainWindow
, and make it a PortfolioListener
so the portfolio can tell it when we add or remove a Sniper.
This makes our top-level code very simple—it just binds together the user interface and sniper creation through the portfolio
:
Even better, since SniperPortfolio
maintains a list of all the Snipers, we can finally get rid of notToBeGCd
.
This refactoring takes us to the structure shown in Figure 17.3. We’ve separated the code into three components: one for the core application, one for XMPP communication, and one for Swing display. We’ll return to this in a moment.
Now that we’ve cleaned up, we can cross the next item off our list: Figure 17.4.
This restructuring of Main
is a key moment in the development of the application.
As Figure 17.5 shows, we now have a structure that matches the “ports and adapters” architecture we described in “Designing for Maintainability” (page 47). There is core domain code (for example, AuctionSniper
) which depends on bridging code (for example, SnipersTableModel
) that drives or responds to technical code (for example, JTable
). We’ve kept the domain code free of any reference to the external infrastructure. The contents of our auctionsniper
package define a model of our auction sniping business, using a self-contained language. The exception is Main
, which is our entry point and binds the domain model and infrastructure together.
What’s important for the purposes of this example, is that we arrived at this design incrementally, by adding features and repeatedly following heuristics. Although we rely on our experience to guide our decisions, we reached this solution almost automatically by just following the code and taking care to keep it clean.
We wrote this refactoring up in detail because we wanted to make some points along the way and to show that we can do significant refactorings incrementally. When we’re not sure what to do next or how to get there from here, one way of coping is to scale down the individual changes we make, as Kent Beck showed in [Beck02]. By repeatedly fixing local problems in the code, we find we can explore the design safely, never straying more than a few minutes from working code. Usually this is enough to lead us towards a better design, and we can always backtrack and take another path if it doesn’t work out.
One way to think of this is the rock climbing rule of “three-point contact.” Trained climbers only move one limb at a time (a hand or a foot), to minimize the risk of falling off. Each move is minimal and safe, but combining enough of them will get you to the top of the route.
In “elapsed time,” this refactoring didn’t take much longer than the time you spent reading it, which we think is a good return for the clearer separation of concerns. With experience, we’ve learned to recognize fault lines in code so we can often take a more direct route.
We did encounter one small bump whilst working on the code for this chapter. Steve was extracting the SniperPortfolio
and got stuck trying to ensure that the sniperAdded()
method was called within the Swing thread. Eventually he remembered that the event is triggered by a button click anyway, so he was already covered.
What we learn from this (apart from the need for pairing while writing book examples) is that we should consider more than one view when refactoring code. Refactoring is, after all, a design activity, which means we still need all the skills we were taught—except that now we need them all the time rather than periodically. Refactoring is so focused on static structure (classes and interfaces) that it’s easy to lose sight of an application’s dynamic structure (instances and threads). Sometimes we just need to step back and draw out, say, an interaction diagram like Figure 17.6:
Our chosen fix relies on the SniperPortfolio
holding onto the reference. That’s likely to be the case in practice, but if it ever changes we will get transient failures that are hard to track down. We’re relying on a side effect of the application to fix an issue in the XMPP code.
An alternative would be to say that it’s a Smack problem, so our XMPP layer should deal with it. We could make the XMPPAuctionHouse
hang on to the XMPPAuction
s it creates, in which case we’d to have to add a lifecycle listener of some sort to tell us when we’re finished with an Auction
and can release it. There is no obvious choice here; we just have to look at the circumstances and exercise some judgment.
3.138.123.106