Refactoring Workbook

A. Smells

Long Method

• Several methods in Table are too long, especially with the action code embedded. It's hard to see where layout ends and action begins.

• Card has similar problems. The constructor in particular seems long.

Large Class

• Both Table and Card seem to be doing too much. (There's no separate model, so these classes act as model, view, and controller.)

Duplicated Code

• Code for setting up all the buttons is similar.

• Each action seems to duplicate some of the work in maintaining the selection.

• Both Table and Card set the border color .

• In Card, the title and the text have similar setups.

• String names of costs appear several times.

• Spaces surround each cost name . (Perhaps the label could be designed to have the space.)

• I'm sure I'll find more once I clean some things up.

Divergent Change

• You can't change the GUI and the actions separately; you couldn't run easily without a GUI.

Shotgun Surgery (one change requires touching several areas)

• The highlighting code (the card border) is in two separate objects.

Feature Envy

• Again, highlighting belongs to Card but Table seems to want in on it.

• Cost information is computed in Table, but it does this by pulling apart what's in the Background.

• The summary and the plan report, as well as several of the actions, also pull information out of the Background.

Data Clumps

• The costLabel and the cost variable seem to go together.

• Lastx/lasty , x/y , getX()/getY() deal with paired values.

Primitive Obsession

• Int for x/y

• Int for velocity

• Strings for BorderLayout (there are constants defined)

• Strings for the cost label

Switch Statements

• The rotateCost() method uses an if statement that is practically a switch.

Lazy Class

• The Background is a pretty small class. I could see making some of the setup be the caller's responsibility. I'm not ready to label this a Lazy Class though; the Feature Envy of Table for Background's components makes me expect some methods will move over here.

Speculative Generality/Dead Code

• There are a couple unused variables : budget and tabs .

Temporary Field

• The selection (in Table) has a little of this feel. (Sometimes it's set, sometimes it's null.)

Inappropriate Intimacy

• Table looks into Background's component list.

• Card knows a bit about its Background (it uses this information in moveToFront() ).

What else?

• The names need some work. Table is especially bad. UpdateCost() is updating a summary of more than just cost.

• The ContainerListener business seems awkward (as does all of selection).

• Each action calls updateCost() to update the summary section. (Could the event model handle this more cleanly?)

• Counting the cards and the rollover count (to set the initial location) is also awkward. Again ”how much configuration should be inside a component, and how much outside?

• There's no separate model. At some level, I can accept that ”this is trying to be a simple implementation. But we're already to the point where that seems troublesome (e.g., the summary updates).

• There are magic numbers (and colors and strings) in various places.

• The buttons could go into a Box instead of the combination of panels it uses now.

When looking for various smells, we keep returning to the same issues.This is partly the result of the smells not being completely orthogonal, but it's also that the same problem can give off different smells.

B. My planned strategy for fixing this:

• Do trivial things first: fix names, long methods, magic numbers, and such.

• Move things related to Background over to that class.

• See what duplication we can eliminate.

• Straighten out selection. This will involve moving features between classes and might introduce new classes.

• Assess whether cost and velocity should be their own classes.

• See where things are, but I'd expect to split the model out.

From easy (low impact) on up:

• Test the classes as is. For example, you could call rotateCost() and verify the resulting cost() .

• Develop methods that walk the components tree to find particular components. For example, you could find the New button, call its doClick() method, and then verify that the body had one more Card.

• Expose the instance variables of the classes so the tests can more directly access components.

• Restructure the application for better testability. For example, have a separate, easily tested model, along with a thin, trivial GUI layer.

Using the simplest approach, which was to create objects as is and call public methods, the test class tests at least something for each object. This test is in the code in Part 2 of this chapter.

Variables budget and tabs are unused.

B. You'll often find it helpful to change inner classes to separate classes; it can make it easier to see duplication.

The result is the starting point for Part 2, the code is online.

Before we move features around, we want to make sure our tests will warn us if we've done so incorrectly. My tests so far have called only publicly available methods, and this didn't include the ability to simulate button clicks. If your tests don't have this ability, add it. You can expose the buttons one by one, or you might find a more generic approach; then call doClick() .

Add the extra tests. I made the buttons have package-level access so the test could get to them. I didn't handle the dialog for the Plan button.

I made Cost have a list of its possible values, with each value showing its name and cost as well as whether it needs an estimate or to be split.

It's easy to extract a method that creates each button and adds it to a panel.

I used Encapsulate Fields on the selection in PlanningGame, so the setter and getter were the only things accessing the selection. Then it was easy to Move Field over to Background. In moving the listener over, I left the guard part on the button and made the actions assume they were called appropriately. For the ContainerListener, I made Background become a ContainerListener rather than maintain a separate class. (This could have gone either way.) Finally, I went through and made methods private where I could.

This requires PlanningGame to have a listener, but it lets us eliminate the updateCost() calls.

I added a parameter to makeButton() to tell whether the button was sensitive to the number of cards.

This was a little trickier than I expected ”there are already other property change notifications when the border is changed. I used a new property name and made Background listen for those notifications.

I restructured the resetSelection() handling, reduced some duplication in the actions for creating a new card and splitting a card, and did a few other bits of polishing.

F. It's an improvement in my eyes. Now the GUI and the model can change independently.

A. The proposed code is faster when the cost of the string comparison is less than the cost of the notification.

B. It can help prevent subtle loops where setting the model notifies a listener, which updates a field, which notifies a listener that updates the model, and so on, in an infinite loop.