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.
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.)
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 .
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.
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.