Clean Code, part 2

In this multipart series, I take some perfectly working code which is definitely not clean and refactor it into a much cleaner and more readable implementation. If you disagree with my approach at any point then I ask that you voice your concerns in the comment section. In part 1 of this series, I introduced what I see as the problem and the code that I will be refactoring in subsequent posts. In this post, part 2 of the series, we make the code a little more readable by refactoring it a little. A lot of work will still be left to be done, however.

After listing the code in part 1 (which I will not list again here in the interest of space), I asked several questions. Let's start by looking at the answers to these questions:

• At a high level, what does this code do?
• It simulates taking a turn in a hangman-like game.
• How many errors can be made before losing?
• 5
• What are lines 36-52 doing?
• Composing a response message to be sent back to the game driver.
• What is the point of the instance variable partialSolution?
• It keeps track of the letters that have been guessed (and not guessed) so far.

I'm skipping the final question about enhancing the functionality of the game since the basic answer to it is "it would be very difficult". The less basic answer would be quite lengthy and out of the scope of this post. We'll return to it later in the series.

How long did it take you to figure out the answers to these questions? My guess is that it took a while. At the very least, you had to go through most of the code to answer the first two questions (and possibly the last one as well). Well written code should not require you to read through most or all of it in order to figure out what it does at a high level. Breaking the code up into functions will help, so we will begin there.

All of the functionality of this code is contained in a single function. How many different things does this function do? Well, it starts out by building the partial solution if it is currently null. (This should happen only the first time through, but it counts as something the function does.) The second thing it does is validate the input. Third, it checks to see if the guess has already been tried and if not then it adds the guess to the list of attempted guesses. Fourth, it checks whether the guess matches any letters in the solution or not. Fifth, it updates the partial solution. Sixth, it figures out the return message stating how many letters were matched by the guess. Finally, it adds a message to the return value explaining whether the player gets to make another guess and, if not, whether the player won or lost.

That one function does SEVEN things (and this is a conservative count). This seems a rather extreme violation of the Single Responsibility Principle. The least we could do is break it into functions that perform each of these steps. Our new code block may look something like this:

[pre class="brush:java" title="GamePlayer.java"]package com.bitsnibblesandbytes.hangman;

public class GamePlayer {

    private static final int MAX_ERRORS = 5;

    private int errors = 0;

    private String guesses = "";

    private String partialSolution;

    public String takeTurn(char guess, String target) {
        createPartialSolution(target);
        if (isInvalidInput(guess)) {
            return "Error: you can only guess letters.";
        }
        if (isAlreadyGuessed(guess)) {
            return "You already guessed that";
        }
        int count = 0;
        if (isCorrectGuess(target, guess)) {
            count = updatePartialSolution(target, guess);
        } else {
            errors++;
        }
        return buildOutputMessage(target, guess, count);
    }

    private String buildOutputMessage(String target, char guess, int count) {
        String feedbackMessage = buildFeedbackMessage(guess, count);
        String winLoseOrContinueMessage = buildWinLoseOrContinueMessage(target);
        return feedbackMessage + "\n" + winLoseOrContinueMessage
    }

    private String buildWinLoseOrContinueMessage(String target) {
        String winLoseOrContinue = "";
        if (errors > MAX_ERRORS) {
            winLoseOrContinue += "The word is " + target + "\nYou lose.";
        } else if (partialSolution.indexOf('*') < 0) {
            winLoseOrContinue += "The word is " + partialSolution + "\nYou win!";
        } else {
            winLoseOrContinue += partialSolution;
        }
        return winLoseOrContinue;
    }

    private String buildFeedbackMessage(char guess, int count) {
        String verb;
        String pluralModifier;
        String number;
        if (count == 0) {
            number = "no";
            verb = "are";
            pluralModifier = "s";
        } else if (count == 1) {
            number = "1";
            verb = "is";
            pluralModifier = "";
        } else {
            number = Integer.toString(count);
            verb = "are";
            pluralModifier = "s";
        }
        return String.format("There %s %s %s%s", verb, number, guess, pluralModifier);
    }

    private int updatePartialSolution(String target, char guess) {
        int count = 0;
        for(int i = 0; i < target.length(); i++) {
            if (target.toLowerCase().charAt(i) == Character.toLowerCase(guess)) {
                count++;
                partialSolution = partialSolution.substring(0, i) + Character.toLowerCase(guess) + partialSolution.substring(i + 1);
            }
        }
        return count;
    }

    private boolean isCorrectGuess(String target, char guess) {
        return target.toLowerCase().indexOf(Character.toLowerCase(guess)) > -1;
    }

    private boolean isAlreadyGuessed(char guess) {
        if (guesses.indexOf(Character.toLowerCase(guess)) >= 0) {
            return true;
        }
        guesses += Character.toLowerCase(guess);
        return false;
    }

    private boolean isInvalidInput(char guess) {
        return !Character.isAlphabetic(guess);
    }

    private void createPartialSolution(String target) {
        if (partialSolution == null) {
            StringBuilder builder = new StringBuilder();
            for (int i = 0; i < target.length(); i++) {
                builder.append('*');
            }
            partialSolution = builder.toString();
        }
    }
}
[/pre]
(Side note: The above code adds a package statement and removes the unused import that was in the previous code in addition to the discussed changes.)

The above code is still far from perfect. For one thing, it is still not particularly object-oriented (since the logic has not been altered at all, just pulled out into private methods), but at least the original method is more readable. The names of the new methods explain what they're doing, which helps with the high-level interpretation of the code. The mystery of the maximum number of errors is also dispelled through the use of the MAX_ERRORS constant, eliminating the use of a magic number in buildWinLoseOrContinueMessage(). The mystery of lines 36-52 in the original code is solved by the name of the buildFeedbackMessage() method. The purpose of partialSolution is still not particularly clear, though the updatePartialSolution() method implementation and the placement of the call to it in the code provide strong clues.

We have begun the process of making this code more readable and more maintainable, but we still have a ways to go. In part 3 of this series, we will explore a more object-oriented approach to the problem and again refactor this code to follow object-oriented principles more closely.