Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm implementing a Java version of the game Mastermind.

My version uses numbers to represent colors, for example 2315. This function compares a string of numbers to the secret code the object stored. It works, but I'm wondering if this could be done in a more elegant way.

public Answer evaluate(String guess) {
    boolean[] codeUsed = new boolean[code.length()];
    boolean[] guessUsed = new boolean[guess.length()];

    int correct = 0;
    int match = 0;

    // Compare correct color and position
    for (int i = 0; i < code.length(); i++) {
        if (code.charAt(i) == guess.charAt(i)) {
            correct++;
            codeUsed[i] = guessUsed[i] = true;
        }
    }

    // Compare matching colors for "pins" that were not used
    for (int i = 0; i < code.length(); i++) {
        for (int j = 0; j < guess.length(); j++) {
            if (!codeUsed[i] && !guessUsed[j] && code.charAt(i) == guess.charAt(j)) {
                match++;
                codeUsed[i] = guessUsed[j] = true;
                break;
            }
        }
    }

    return new Answer(guess, correct, match, guess.length() - correct - match);
}
share|improve this question
    
Why do you return guess also? And what is guess.length() - correct - match? –  abuzittin gillifirca Jan 23 '13 at 16:22
    
The Answer is used to print out the results for a given attempt. Its toString() method for example returns a string "1234 -> 0 correct, 2 match, 2 wrong". guess.length() - correct- match are the wrong pins. –  Sven Jan 23 '13 at 16:34

3 Answers 3

The algorithm is short and concise. There might be a more clever way to do it, but it would likely be less readable.

This code will break if the guess is not the same length as the code. An empty string causes an out of out of bounds exception. A string longer than the code could cause other problems. You don't show what Answer is, but it looks like the last argument is the number of incorrect pins. This could lead to having more incorrect pins than there are in the code. You need to protect against each of these cases.

Since you are using strings, the value could contain characters other than digits. A guess with invalid characters should be treated differently than an incorrect guess.

You can add a check before the second loop to see if the all of the characters are correct. In that case you would skip over a N x M loop.

share|improve this answer
    
Those checks are done in another method which returns status messages (result for given string, remaining tries, Conratulations you won, wrong input). Should those checks be performed additionally in this method? –  Sven Jan 23 '13 at 16:37

There's an alternative and slightly simpler algorithm that works by counting the number of times each color appears in the code and in the answer. The total number of pins (white and black) in the evaluation is then given by the sum (over all colors) of whichever is the smaller of these. Here's how you might implement it (I've written COLORS for the number of different colors):

public Answer evaluate(String guess) {
    // Total number of pins in the answer.
    int pins = 0;

    // Number of black pins in the answer.
    int black = 0;

    // Number of times each color appears in the code and the guess.
    int[] codeColorCount = new int[COLORS];
    int[] guessColorCount = new int[COLORS];

    for (int i = 0; i < code.length(); i++) {
        char c = code.charAt(i);
        char g = guess.charAt(i);
        ++ codeColorCount[c.getNumericValue()];
        ++ guessColorCount[g.getNumericValue()];
        if (c == g) {
            ++ black;
        }
    }

    for (int i = 0; i < COLORS; i++) {
        pins += Math.min(codeColorCount[i], guessColorCount[i]);
    }

    return new Answer(guess, black, pins - black, guess.length() - pins);
}

(Since the numbers in codeColorCount never change during a game, you might decide to cache them.)

In some languages this algorithm can be expressed tersely, for example in Python you could write it like this:

from collections import Counter
def evaluate(code, guess):
    pins = sum((Counter(code) & Counter(guess)).values())
    black = sum(c == g for c, g in zip(code, guess))
    return guess, black, pins - black, len(guess) - pins
share|improve this answer
boolean[] codeUsed = new boolean[code.length()];
boolean[] guessUsed = new boolean[guess.length()];
codeUsed[i] = guessUsed[i] = true;
if (!codeUsed[i] && !guessUsed[j] && ...) {
codeUsed[i] = guessUsed[j] = true;

I can not see why both of them are used? You always set both of them to the same value and access both of them. So one can safely be removed.

return new Answer(guess, correct, match, guess.length() - correct - match);

In general, you should try to reduce the number of arguments. In this case, you do not need the fourth argument, because you can easily calculate it inside the Answer object.

for (int i = 0; i < code.length(); i++) {
    for (int j = 0; j < guess.length(); j++) {
        if (!codeUsed[i] && !guessUsed[j] && code.charAt(i) == guess.charAt(j)) {
            match++;
            codeUsed[i] = guessUsed[j] = true;
            break;
        }
    }
}

You do not need to keep track of the already used parts. Just do a match = correct - matchor something like this after you have counted all matches. To simplify this, i would count the appearance in a temporary string, which is reduced for every hit.

I would add some error checking for the guess argument. Could be helpful if something weird happens.

All combined, it could look like this:

public Answer evaluate(final String guess) {
    if (guess == null || guess.isEmpty() || guess.length() != code.length())
        throw new IllegalArgumentException("Invalid input. guess: " + guess + ", code: " + code);

    int correct = 0;
    int matchAll = 0;

    String reducedString = guess;
    for (int i = 0; i < code.length(); i++) {
        if (code.charAt(i) == guess.charAt(i))
            ++correct;

        final String currentCharAsString = String.valueOf(code.charAt(i));
        if (reducedString.contains(currentCharAsString))
        {
            ++matchAll;
            reducedString = reducedString.replaceFirst(Pattern.quote(currentCharAsString), "");
        }
    }
    return new Answer(guess, correct, matchAll - correct);
}

After all, you should write some unit tests to check the code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.