CPSC 329 Lab 5: Klondike Code Review

This code review is about maintainability — the findings listed below deal with code smells rather than bugs.

1. Unclear names in TCol and Game

Smell: Mysterious Name

Where:

• TCol (class, and its test class TColTest)
• Game.proc(int a, int b) and its local variables x and y
• local variable t in Game.play()
• TCol.getTop()

Problem: None of these names says what the thing is or does. TCol is an abbreviation readers have to decode. proc could be any method at all, and a/b/x/y don't say which column is the source and which is the destination. t is the player's command. getTop() is worse than unclear: it sounds like a getter, but it removes the top card. As a result, the line column.getTop(); in moveColumnToFoundation looks like it does nothing.

Fix: Rename:

• TCol → TableauColumn (and TColTest → TableauColumnTest)
• proc → moveColumnToColumn, a → fromCol, b → toCol, x → from, y → to
• t → command
• getTop → removeTop

Done when: the renames are made everywhere, the Javadoc comments match the new names, and all tests pass.

2. Game.play() is too long

Smell: Long Method (with Comments as Deodorant and Complicated Boolean Expression)

Where: Game.play()

Problem: play() is about 150 lines long and does everything: printing the board, reading the command, carrying out each command, and checking for a win. The comments // print the board, // get the player's command, etc. are a sign that the method is really several methods. The win check is a long && expression that has to be read carefully to see what it means.

Fix: Extract methods from play():

• printBoard(), from the board-printing code
• readCommand(), which prompts for and returns the command
• drawCard(), from the body of the d command
• isWin(), from the win-check condition

Then delete the section comments. The method names say the same thing. (The long w, c, and m branches are the subject of finding 3.)

Done when: play() reads as a short summary of a turn, each extracted method has a Javadoc comment, and all tests pass.

3. Column input loop is copied four times

Smell: Duplicate Code

Where: Game.play(): the loop that asks for a column number and reprompts until it gets a legal one appears in the w command, the c command, and twice in the m command (once for each column).

Problem: Four copies of the same 15 lines means four places to update if, for example, the prompt or the number of columns changes. Copies also tend to drift apart over time.

Fix: Extract the loop into a method readColumn() that returns the column number, and replace the copies with calls to it.

Done when: play() has no copies of the input loop, and all tests pass.

4. Game.isLegalTableauMove belongs in TCol

Smell: Feature Envy (with Complicated Boolean Expression)

Where: Game.isLegalTableauMove(TCol col, Card card)

Problem: This method only uses the column's data and none of Game's. It reaches into col.cards_ five times. The question "can this card go on this column?" belongs to the column. The expression col.cards_.get(col.cards_.size() - 1) is also repeated three times in one return statement, which makes the rule hard to read.

Fix: First give the repeated expression a name (a local variable for the column's top card). Then move the method into the column class, so the call becomes column.isLegalTableauMove(card).

Done when: the method is in the column class and is public with an accurate Javadoc comment, Game calls it on the column, and all tests pass.

5. TCol and Foundation use different names for the same operations

Smell: Alternative Classes with Different Interfaces

Where: TCol and Foundation

Problem: Both classes are piles of cards that can check whether a card can be played on them, add a card, and show the top card. But they use different names for these operations, so code that works with one kind of pile can't be reused for the other, and readers have to learn two vocabularies:

Fix: Rename so both classes use canAccept(Card), add(Card), top(), and isEmpty(). Then extract an interface Pile with those four methods, and have both classes implement it.

Done when: both classes implement Pile, and all tests pass.

6. Game does too many jobs

Smell: Large Class (with Divergent Change)

Where: Game

Problem: Game holds the game state, enforces the rules, reads the player's input, and prints everything. It has to change for unrelated reasons: a rule change and a user-interface change both mean editing Game. When we add a GUI, all of the console code tangled into Game will get in the way.

Fix: Start pulling the console user interface out of Game. Extract a class ConsoleUI that holds the Scanner, and move the input methods (readCommand() and readColumn(), from findings 2 and 3) and printHelp() into it. Moving the rest of the output (printBoard() and the messages in the move methods) is a bigger job. File it as a separate follow-up issue rather than doing it here.

Done when: Game has no Scanner field, ConsoleUI has readCommand(), readColumn(), and printHelp(), a follow-up issue for the remaining output has been filed, and all tests pass.

7. Game.reportMove has a long parameter list

Smell: Long Parameter List (with Data Clumps)

Where: Game.reportMove(Card card, String fromPile, int fromNum, String toPile, int toNum, boolean verbose) and its four callers

Problem: Six parameters are hard to get right at each call (which int is which?). verbose isn't used at all, and every caller passes true. The other five values always travel together, and together they describe one thing: a move.

Fix: Remove the unused verbose parameter. Then introduce a parameter object: a Move class holding the card and the from/to piles, so the method becomes reportMove(Move move). (A Move class would also be useful later for undo or a move history.)

Done when: reportMove takes a single Move, and all tests pass.

8. Game reaches inside TCol's list of cards

Smell: Insider Trading

Where: TCol.cards_ is public, and Game uses it directly in the constructor (dealing) and in proc (moving cards between columns)

Problem: Any class can change a column's cards in any way. That includes breaking the rules or leaving a face-down card on top. And TCol can't change how it stores its cards without breaking Game.

Fix: Make cards_ private. Replace Game's direct uses with TableauColumn methods, using existing ones where they fit (e.g. add, top) and adding new ones where needed (e.g. finding the card that can move to another column, and moving a run of cards). Don't replace the field with a public getter for the list: that just hides the same problem.

Done when: cards_ is private, no other class touches a column's list, and all tests pass.

9. Magic numbers

Smell: Magic Number

Where:

• Game and the column input code: 7 (number of tableau columns), 4 (number of foundations), 13 in the win check (cards in a complete foundation)
• TCol.isLegalTableauMove (after finding 4): 13 (rank of a king)
• Foundation.canAccept: 1 (rank of an ace)

Problem: A reader has to work out what each number means, and changing one (say, for a variant with a different number of columns) means finding every copy.

Fix: Replace each with a named constant, e.g. NUM_COLUMNS, NUM_FOUNDATIONS, CARDS_PER_SUIT, KING, ACE. Constants that are about cards (ACE, KING) belong in Card. Careful: not every 1 in Foundation means "ace".

Done when: the numbers above are named constants, and all tests pass. (The scoring numbers are covered by finding 10.)

10. Scoring rules are spread across classes

Smell: Shotgun Surgery

Where: Game changes score_ directly in five different methods (turning the waste over, waste to column, waste to foundation, column to foundation, and column to column), and TCol.revealTop() returns the points for turning over a card

Problem: Changing the scoring, for example to add "Vegas" scoring as an option, means edits in many places in two classes, and it's easy to miss one. A column also shouldn't need to know how the game is scored.

Fix: Collect the scoring rules in a Score class with a method for each scoring event (e.g. wasteToTableau(), toFoundation(), cardTurnedOver(), wasteRecycled()) and getPoints(). Game calls these methods, and revealTop() returns whether a card was turned over instead of a number of points.

Done when: all of the point values are in Score, and all tests pass (updating the tests of revealTop for its new return type).

11. Game has forwarding methods that add nothing

Smell: Middle Man

Where: Game.foundationCanAccept(int, Card) and Game.addToFoundation(int, Card)

Problem: Each of these just calls the same-named method on foundations_[i]. They're an extra layer to read through without adding anything.

Fix: Inline both methods.

Done when: both methods are gone, and all tests pass.

12. Unused code

Smell: Dead Code

Where: Game.printDebugState() and TCol.countFaceUp()

Problem: Neither method is called anywhere. Unused code still has to be read, understood, and kept up to date when other things change.

Fix: Confirm that nothing uses them, then delete them. (Version control remembers them if they're ever needed again.)

Done when: both methods are gone, and all tests pass.