6
\$\begingroup\$

The evaluate_guess function below returns the evaluation of a guess with respect to Mastermind game rules:

# sample secret code and guess
secret_code = ["a", "a", "c", "a"]
guess = ["c", "a", "a", "b"]


def evaluate_guess(secret_code, guess):
    score = []
    temp_list = secret_code.copy()

    for i in range(len(secret_code)):
        if guess[i] == temp_list[i]:
            score.append(1)
            temp_list[i] = None

        elif guess[i] in temp_list:
            score.append(0)
            temp_list[temp_list.index(guess[i])] = None

    score.sort(reverse=True)
    return score

print(evaluate_guess(secret_code, guess))

>>> [1, 0, 0]
  1. Without changing the essence of the algorithm: Can I implement the evaluate_guess function in a more Pythonic way? Is it possible to implement the algorithm in a purely functional fashion?

  2. Is there a more straightforward algorithm?

\$\endgroup\$
3
  • 3
    \$\begingroup\$ This isn't functional-programming, it's imperative. \$\endgroup\$
    – Peilonrayz
    Nov 23, 2015 at 23:33
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ Nov 24, 2015 at 23:18
  • 3
    \$\begingroup\$ Hello there, I rolled back the latest edit. I strongly recommend you read the link provided by TheCoffeeCup in the previous comment, since that explains why they rolled back and also why I rolled back. Please don't edit your question in that way. If you have further questions, feel free to drop by in Code Review Chat \$\endgroup\$
    – Vogel612
    Nov 24, 2015 at 23:27

4 Answers 4

4
\$\begingroup\$

Most of the solutions provided so far is hard to read, and you need to really think before you understand what is happening, so I would like to propose an alternative approach as a response to a more straightforward algorithm:

def evaluate_guess(secret_code, guesses):

    score = []
    remaining_secrets = []
    remaining_guesses = []

    # Check exact matches
    for guess, secret in zip(secret_code, guesses):
        if guess == secret:
            score.append(1)

        else:
            remaining_guesses.append(guess)
            remaining_secrets.append(secret)

    # Check wrong placing
    for guess in remaining_guesses:

        if guess in remaining_secrets:
            score.append(0)
            remaining_secrets.remove(guess)

    return score

The only magic in this code is the joining of secret_code and guesses using zip which joins them index for index. This allows for, in my opinion, code that is easier to understand instead of list comprehensions, ifilters, index searching, sorting, sliceing and other stuff.

\$\endgroup\$
1
  • \$\begingroup\$ Yes, your code is both straightforward and easy to read. \$\endgroup\$
    – blackened
    Nov 25, 2015 at 10:08
3
\$\begingroup\$

If this were functional programming, then temp_list[i] = None should never happen.

And so, you would be able to do:

# Ugly Python functional programming.
return list(sorted(map(
    lambda i: guess[i] == secret_code[i]
    filter(lambda i: guess[i] in secret_code, range(len(guess)))
)), reverse=True))

Or using a comprehension:

return list(sorted((
    guess[i] == secret_code[i]
    for i in range(len(guess))
    if guess[i] in secret_code
), reverse=True))

But it is not.

Your algorithm is actually really good.

The best way to make it more Pythonic, is to use enumerate. That way, you are both generating i, and indexing guess at the same time.

Also, you have a bug. This is as you remove some future used items with your temp_list that uses index. For an example if you input ['a', 'b', 'a'], ['a', 'a', 'a'], you get [1, 0] not [1, 1].

def evaluate_guess(secret_code, guess):
    secret_code = secret_code.copy()
    ones = [
        1
        for secret, guess_item in zip(secret_code, guess)
        if secret == guess_item 
    ]

    zeros = []
    for guess_item in guess:
        if guess_item in secret_code:
            secret_code[secret_code.index(guess_item)] = None
            zeros.append(0)

    return ones + zeros[:-len(ones)]

evaluate_guess(["a", "a", "c", "a"], ["c", "a", "a", "b"])
#[1, 0, 0]

Note: this algorithm is \$O(n^2)\$ and so you may want to change it to a more \$O(n)\$ algorithm if you need to support larger numbers.

\$\endgroup\$
2
  • \$\begingroup\$ Excellent catch. Changed my unpythonic code accordingly. \$\endgroup\$
    – blackened
    Nov 24, 2015 at 23:10
  • \$\begingroup\$ I appreciate the contrast between the paradigms. I think it's important to outline that different languages work differently and encourage different practices. \$\endgroup\$
    – J-Cake
    Aug 12, 2020 at 4:03
1
\$\begingroup\$

A more pythonic way to do this would be to use list comprehensions:

def evaluate_guess(secret_code, guess):
    score1 = [1 for i in range(len(secret_code)) if guess[i] == secret_code[i]]  ## list for exact matches
    score2 = [0 for i in range(len(secret_code)) if guess[i] in secret_code]   ## list for "color" matches
    score1.extend(score2[:-len(score1)])
    score1.sort(reverse=True)  
    ## in this method, every 1 also shows up as a zero, so when you combine the lists, you just account for that by subtracting a zero for every one that appears
    return score1

It's up to you whether this is more "straightforward". Some people like list comprehensions, and for others (like me) I find them a little harder to read because I'm still not that used to them.

In this version, you don't need to keep up with the temp_list, and you also don't need the step of initializing the lists, because they are created in a single step. (so, fewer lines of code)

\$\endgroup\$
9
  • \$\begingroup\$ I suspect your algorithm is erroneous, especially score2 = [0 for i in range(len(code)) if guess[i] in code]. \$\endgroup\$
    – blackened
    Nov 23, 2015 at 22:57
  • \$\begingroup\$ I did find a bug where I mixed in some Java...need to do the sort on a separate line. When I run this code with your inputs, I get your desired output. \$\endgroup\$
    – gariepy
    Nov 23, 2015 at 23:06
  • \$\begingroup\$ @blackened - I tried more test cases, including all correct, and none correct, and it works. Are you having a problem with it? \$\endgroup\$
    – gariepy
    Nov 23, 2015 at 23:11
  • \$\begingroup\$ Why using i in range(len(code)) ? For 1 you can use for c,g in zip(secret_code, guess) if c == g and for 0 for g in guess if g in secret_code. Besides len(code) is a NameError. \$\endgroup\$ Nov 24, 2015 at 7:37
  • \$\begingroup\$ After reading this again (and not being half asleep) I think this is the best solution, +1. I think you should add more of a review of OP's code and explain the algorithm a little. E.g. for every guess[i] == secret_code[i], guess[i] in secret_code is true. \$\therefore \text{score1} \subset \text{score2}\$ and so score2[:-len(score1)] removes the duplicates. \$\endgroup\$
    – Peilonrayz
    Nov 24, 2015 at 10:39
0
\$\begingroup\$

This doesn't count as functional programming (which isn't really Python's way), but if I were to code this in Haskell, for example, I would do something like this:

def evaluate_key(secret_code, key, position):
    # Will return a None value in case of a "miss", as per your plan.
    if key == secret_code[position]:
        return 1
    elif key in secret_code:
        return 0

def evaluate_guess(secret_code, guess, secret_code_length=4):
    return sorted(
        filter(None.__ne__, # Filter out None values. See https://docs.python.org/3.5/reference/datamodel.html#object.__ne__
            [evaluate_key(secret_code, g, p) 
            for (g, p) in zip(guess, list(range(secret_code_length)))]
        ),
        reverse=True
    )

The plan would be to zipWith the guess list and a list of positions ([0, 1, 2..]) with the function evaluate_key and then sort it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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