Sei

Refactoring more.

Today, I turned this:

private int getValidPosition(Board board) {
        output.println("Please choose a number between 1-9");
        while (true) {
            int number = getNumber();
            if (isPositionInBounds(number)) {
                if (isPositionMarkable(number, board)) {
                    return number;
                } else {
                    output.println("That cell is already marked, try again");
                }
            } else {
                output.println("That is not a valid position");
            }
        }
    }

    private boolean isPositionInBounds(int number) {
        return (number >= 1 && number <= 9);
    }

    private boolean isPositionMarkable(int number, Board board) {
        return (board.isCellEmpty(number));
    }

    private int getNumber() {
        int number;
        while (!input.hasNextInt()) {
            output.println("That is not a valid input");
            input.next();
        }
        number = input.nextInt();
        return number;
    }

Into this:

private int getValidPosition(Board board) {
        output.println("Please choose a number between 1-9");
        while (true) {
            int position = getNumber();
            if (outOfBounds(position) || alreadyMarked(board, position))
                continue;
            return position;
        }
    }

    private boolean outOfBounds(int number) {
        if (!isPositionInBounds(number)) {
            output.println("That is not a valid position");
            return true;
        }
        return false;
    }

    private boolean alreadyMarked(Board board, int number) {
        if (!isPositionMarkable(number, board)) {
            output.println("That cell is already marked, try again");
            return true;
        }
        return false;
    }

    private boolean isPositionInBounds(int number) {
        return (number >= 1 && number <= 9);
    }

    private boolean isPositionMarkable(int number, Board board) {
        return (board.isCellEmpty(number));
    }

    private int getNumber() {
        while (!input.hasNextInt()) {
            output.println("That is not a valid input");
            input.next();
        }
        return input.nextInt();
    }
}

I am not convinced that I have got it nailed yet. Since I think that the method names, although clear, seem to have duplication in them “outOfBounds” and “isPositionInBounds”. I haven’t yet decided if the solution is to rename the methods to make them more suitable, one is obviously more focused on alerting the user that their choice was a poor one, the other is really determining whether that is the case. Alternatively, maybe there is a sensible way to combine them into one method, without making the “getValidPosition” method any larger than it needs to be.

More broadly I think I have fallen into the trap of primitive obsession elsewhere in my code, since my main data structure for the Board Class is a char[]. I’ll need to look further into this code smell to understand whether I have fallen into that trap, or if not, how to avoid it.

There seems to be an overwhelming amount to learn at the moment, and I need to stop getting frustrated when I can’t work something out. I think I have an idea for a blog post at some point in the future, working title “How to suceed as an Apprentice at 8th Light”. Perhaps theres benefit associated in coming at the apprentiship without a preconceived idea about how it will be, what to expect etc. However, I think there may be some bonus in knowing what worked for other people. I’ll think on it, and see how I might structure it for later.