Part 2 : Redistributing Features


Part 2: Redistributing Features

The story so far : We have a planning game simulator built around three classes ”PlanningGame (managing the controls and the background), Background (a playing surface), and Card (for the index cards). We identified a number of places that could be improved, created a few simple tests, and did some simple refactorings. The starting-point code for this part is available at www.xp123.com/rwb/ .

There were several places in PlanningGame (formerly Table) that accessed fields inside Background. This is a case of Feature Envy: A class spends time manipulating data that is somebody else's responsibility. The cure is to move data and methods around.

Exercise 98 Test the Buttons .

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 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; once you have a button, you can call doClick() to make it do its thing.)

See Appendix A for solutions.


Notice how the Plan button causes trouble: It wants to create a dialog, which is a pain to test programmatically. I'll just leave it for now and be extra careful when working on it. (This is one of those places where you want to refactor to enable you to create a test, but you want a test to help you to refactor safely.)

Exercise 99 Move Features to Background.
  • Extract the "string computation" part of the Plan button and move it to Background. (You may have to test it manually, but you should be able to automate a test after the refactoring.)

  • Move cost() to Background.

  • Create a count() method on Background that looks at getComponentCount() ; adjust the callers .

  • Create a top() method on Background that returns getComponent(0) and adjust the callers. (I'd try returning something of type Card for now because the callers all want to cast it anyway.)

The latter two changes are more in the spirit of improving communication than decoupling.

Exercise 100 Move Features to Card.

The Card doesn't know its own selection status. Change that by pulling over the code for a new method setSelected(boolean) . I had expected to need a method isSelected() , but nobody looks at the selection status!


I'm not fully happy with the selection handling, nor with the fact that the Card knows about its background. But I'm not quite sure how to handle it, and there is still a lot of duplication around the cost in a Card and the button-making in PlanningGame. Since I have ideas for those, I'll tackle them next .

Exercise 101 Clean up Cost.
  • Get rid of those annoying spaces surrounding each label. (It's pretty clear they're just for padding.)

  • Extract a Cost class. Adjust the test classes.

See Appendix A for solutions.


Pulling this class out made me wonder where Card.needsEstimate() and Card.needsSplit() are called. Each is called by Background.planAnalysis() and the tests. But planAnalysis() is asking the Card for those checks; why not let the Card compute its own analysis? (Later, there might be other analyses not at card level, but we'll let that day take care of itself.) So eliminate this Feature Envy by putting a planAnalysis() method on Card. This is a problem I hadn't noticed earlier, but the refactoring made it clear.

Five Whys

While we're looking at planAnalysis() , notice the last argument of JOptionPane.showMessageDialog() . In looking over this exercise, my colleague Edmund Schweppe pointed out that OK_OPTION is not the right constant to pass to that method. That argument is intended to tell whether the message is a warning, informational, etc.; if we talk to our customer, we find out that it should be a warning (if there are any problems) or informational ( otherwise ). It's not unusual to find a bug during refactoring.

Lean manufacturing has the notion of asking why five times to get at the root causes of problems. The outermost symptom is that it always shows the stop sign ”is that really what the user wants? One level back, we see that the wrong constant has been used. But why wasn't it detected ? One reason is that we don't have any tests for dialog types. (Yes, they're a pain to test.) Note also how the code was formatted: one very long line, with the wrong constant at the far right end. In my editor, this means I would never have a chance to see the constant unless I explicitly went to the end of the line. (Personal resolution: Update my personal coding style to format one argument per line on long lines.)

Why was it written wrong in the first place? One reason is that I didn't have documentation at hand; I relied on the editor to suggest constants and grabbed one that sounded right. (Personal resolution: Get a new Java Foundation Classes manual and don't rely on memory so much.) In this case, I was coding solo ”my partner could easily have been looking up the arguments while I was typing.

And why is it possible to make this kind of error? Partly, it's a result of a decision the library designers made: to use only integer constants for all the option types and message types. The library designers could have created separate classes for these options (as a sort of enumeration); or they could have used non-overlapping values or different methods for the different message types. (Personal resolution: Watch out for primitive obsession in my own code, even if I can't do anything about the library's.)

Even a small mistake has the seeds of many lessons.



Refactoring Workbook
Refactoring Workbook
ISBN: 0321109295
EAN: 2147483647
Year: 2003
Pages: 146

flylib.com © 2008-2017.
If you may any questions please contact us: flylib@qtcs.net