Pythons Around the Rose

Question

Based off my Java Petals Around the Rose, I thought it would be good practice to write something again in a different language.

Because the language is Python, I named it "Pythons Around the Rose", and everywhere in my code, the word "rose" would be replaced with "python".

Also, "Potentate" is replaced with "Pytentate".

Code:

import random

def display_help():
    example = get_dice_result()
    print("The name of the game is Pythons Around the Rose.",
          "The name is important. I will roll five dice,",
          "and I will tell you how many pythons there are.",
          "\nFor example:", sep = "\n")
    print_dice(example)
    print("Will result in ", get_answer(example), ".", sep = "")
    print("\nIf you answer correctly 8 times in a row, you",
          "will be declared a \"Pytentate of the Rose\".", sep = "\n")

def play():
    streak = 0
    while True:
        dice = get_dice_result()
        if play_once(dice):
            streak += 1
        else:
            streak = 0
        if streak == 8:
            print('You are now declared a "Pytentate of the Rose"!')
            break;
    print("Thank you for playing!")

def get_dice_result():
    result = [get_dice_roll(), get_dice_roll(),
              get_dice_roll(), get_dice_roll(), get_dice_roll()]
    return result

def get_dice_roll():
    return random.randrange(6) + 1

def play_once(results):
    print("How many pythons here?")
    print_dice(results)
    guess = get_guess()
    answer = get_answer(results)
    if guess == answer:
        print("Correct!")
        return True
    print("Incorrect. The answer is ", answer, ".", sep = "")
    return False

def get_guess():
    guess = 0
    valid = False
    while not valid:
        try:
            guess = int(input("> "))
            valid = True
        except:
            print("\nOops! That is not a number. Try again: ")
    return guess

def get_answer(dice):
    answer = 0
    for i in dice:
        if i == 3:
            answer += 2
        elif i == 5:
            answer += 4
    return answer;

def print_dice(dice):
    rows = ["|", "|", "|"]
    for i in dice:
        if i == 1:
            rows[0] += "       |"
            rows[1] += "   .   |"
            rows[2] += "       |"
        elif i == 2:
            rows[0] += " .     |"
            rows[1] += "       |"
            rows[2] += "     . |"
        elif i == 3:
            rows[0] += " .     |"
            rows[1] += "   .   |"
            rows[2] += "     . |"
        elif i == 4:
            rows[0] += " .   . |"
            rows[1] += "       |"
            rows[2] += " .   . |"
        elif i == 5:
            rows[0] += " .   . |"
            rows[1] += "   .   |"
            rows[2] += " .   . |"
        elif i == 6:
            rows[0] += " .   . |"
            rows[1] += " .   . |"
            rows[2] += " .   . |"
    print(rows[0], rows[1], rows[2], sep = "\n")

def main():
    display_help()
    play()

if __name__ == '__main__':
    main()

I wrote it to be based of standard conventions as close as possible. Because I'm a Java person, I might have added a ; somewhere in my code, or perhaps accidentally forgot to follow conventions somewhere.

Answer 1

In general your code looks nice, but it does show that you've got a another programming language background, and thus you do it the standard imperative way instead of utilizing some of the more pythonic way. In this review I'm deliberately going a little over the top, but in return you get to see some of the more special pythonic constructions.

• Using __doc__ to get the docstring for display_help – In Python you can and should describe your code using docstrings, typically text with triple quotes, i.e. """ ... """. These can be used to describe the module, functions, classes (and methods). They typically come on the line after the definition of the item you want to document.

  A neat thing is that this docstring can be accessed using a special variable, __doc__, or for a function as display_help.__doc__. And then it can be used for print outs, or similar.

• Work a little more on naming methods – It is not very common in Python to have get_xxxx methods, rather name them according to specific function, like roll_multiple_dice() or roll_die(). Aim for simplicity and clarity. Using get_answer() implies that you want me to insert the answer of something, whilst number_of_pythons() indicates that this is going to be calculated.

• List comprehensions are your friend – Instead of repeating code to build the list, use return [roll_die() for _ in range(number_of_dice)] which implements three nice features:

  1. A list comprehension, which builds a list based on the inner generator, i.e. [ ... ]
  2. The use of for within the generator, x for x in range(y), which compresses a for loop into the expression. This one would return the numbers from 0 to y-1
  3. The use of _ when we don't really care about the number, but just want the code in front of the for loop to be executed

• Generator expressions – The for loop of last item, can also be used outside of list comprehension, i.e sum(x for x in range(y)), which would sum all the numbers from 0 through y-1.

  One major difference between a list comprehension and generator, is that the list comprehension actually builds the entire list, whilst the generators works in the background and gives you another element when you want it.

  In the sum example this means that if you had y=1000000 you wouldn't allocate memory for more than 1 int, whilst if you used list comprehension you would allocate the entire list of 1 million elements, and then sum it.

• The Python ternary, i.e. a = b if c else d – This could take some time to get used to, but actually it reads out quite nicely: Set the value of a to b if the condition c is true, or d if not true.

  Disclaimer: In my refactored code this is where it got a little ugly, as I combined multiple of this construction. But still, they kind of make sense. I used them in both number_of_pythons() and print_dice().

• Avoid one-time intermediate variable – It's quite common to skip the intermediate variable, especially if you use them only once, and rather use the expression directly. I.e. in roll_die() just return the random number, and in roll_multiple_dice() return the list directly.

• In some cases, you can sum using Booleans – Doing the if True: a += 1 is kind of an anti-pattern, and can in some cases be replaced by a += True, or in your case: streak += play_once(...).

  • Bug found by 200_success: Here the simplification went too far, as I by simply summing, removed the resetting of the streak counter.

• Avoid flag variables, if possible – Your use of valid in get_guess() is better written using while True and a break like in your input validations. And skipping the intermediate variable, you could return straight out of the loop.

• Try to avoid the chained if commands, doing the same stuff – In both get_answer() and print_dice() you have if chains where the block kind of does the same. This is usually an indication that you could find a better way of handling it.

  • In get_answer() I opted for a double ternary, doing answer += 2 if i == 3 else 4 if i == 5 else 0.

  • In print_dice() I used the fact that a six sided die has only 5 distinct patterns, and then I used yet another double ternary to select from the different options.

    Disclaimer: Even though this code is a lot shorter than yours, that doesn't mean it is nicer as such. But it does show an alternate way, and there are multiple ways to print die results. See here or here.

• Use constants, not magic number – Instead of hiding a 5 and an 8 in the code, use constants at top of file like REQUIRED_STREAK_RUN = 8 and NUMBER_OF_DICE = 5. This allows you to change them easily (and you can still make the document reflect the change using str.format()

• Use str.format() for string formatting - This is a very useful construct, which is well worth some time to look into. You could either use it without names (see end of play_once()), or you could name your variables like in display_help().

  PS! If using Python 2, I recommend using from __future__ import print_function to get the full extent of this beauty there as well.

• Feature: You can't quit your program without becoming a Pythentate – I think it's nice to give users a way out. I've implemented a lazy version with terminating if entering a negative answer. This could be done a lot nicer, but it should be an option to quit at any time.

Refactored code

Here is the code using most (if not all) of the advice from above:

"""Pythons Around the Rose

The name of the game is Pythons Around the Rose.
The name is important. I will roll {number_of_dice} dice,
and I will tell you how many pythons there are.

For example: 

| .   . |       | .   . | .     | .   . |
|   .   |   .   |       |   .   |   .   |
| .   . |       | .   . |     . | .   . |

Will result in 10 pythons. If you answer 
correctly a given {streak_run} number of times you
will be declared a "Pythentate of the Rose".
"""
import random
import sys

REQUIRED_STREAK_RUN = 8
NUMBER_OF_DICE = 5

def display_help():
    """Use the docstring of the module."""
    print(__doc__.format(number_of_dice = NUMBER_OF_DICE,
                         streak_run = REQUIRED_STREAK_RUN))

def play():
    """Play until declared a Pytentate..."""
    streak = 0
    while True:

        # My bug: streak += play_once(roll_multiple_dice(NUMBER_OF_DICE)):
        if play_once(roll_multiple_dice(NUMBER_OF_DICE)):
            streak += 1
        else:
            streak = 0

        if streak == REQUIRED_STREAK_RUN:
            print('You are now declared a "Pytentate of the Rose"!')
            break

    print("Thank you for playing!")

def roll_multiple_dice(number_of_dice):
    """Return a list of die rolls."""
    return [roll_die() for _ in range(number_of_dice)]


def roll_die():
    """Return a single die roll."""
    return random.randrange(6) + 1


def play_once(results):
    """Play a single round, and return True if correct guess."""
    print("How many pythons here?")
    print_dice(results)

    guess = get_guess_of_pythons()

    # Simple bail out option... :-)
    if guess < 0:
        sys.exit(0)

    answer = number_of_pythons(results)
    if guess == answer:
        print("Correct!")
        return True

    print("Incorrect. The answer is {}".format(answer))
    return False


def get_guess_of_pythons():
    """Get input of pythons from user."""
    while True:
        try:
            return int(input("> "))
        except:
            print("\nOops! That is not a number. Try again: ")


def number_of_pythons(dice):
    """Return correct number of pythons."""
    return sum(2 if i == 3 else
               4 if i == 5 else
               0 for i in dice)


EYES = ["       |",
        "   .   |",
        " .     |", 
        "     . |",
        " .   . |"]

def print_dice(dice):
    """Return a three-line string of all the dice."""
    rows = ["|", "|", "|"]
    for i in dice:
        rows[0] += EYES[0] if i == 1 else \
                   EYES[2] if i < 4 else \
                   EYES[4]

        rows[1] += EYES[4] if i == 6 else \
                   EYES[1] if i % 2 == 1 else \
                   EYES[0]

        rows[2] += EYES[0] if i == 1 else \
                   EYES[3] if i < 4 else \
                   EYES[4]

    print('\n'.join(rows))


def main():
    display_help()
    play()

if __name__ == '__main__':
    main()

Conclusion

I hope you are not intimidated by the length of this answer, or the content, because your code does look nice and it does do the work. But there are things to work with to make it more Pythonic, and I hope you've gotten a gist of even more of the beauty of Python here.

Answer 2

I will try to go function by function through your code.

But first a general note on the source code: In Python (and most other programming languages) it is best practice to have a short docstring for each function. A short description what the function does might be sufficient for your case.

display_help():

• When printing your help message, you use the sep-keyword of the print function. PEP8 recommends not to have whitespace around the = sign if you define a keyword argument.
• Python allows you to define multiline strings using enclosing """ ... """, which should preserve linebreaks.
• You should add that help text somewhere as a docstring. Consider the main method or a module docstring. This will make the information available to anyone who might use it as a module instead of a script.

play():

Not much to say here from my point of view, apart from the missing docstring (and the ;). You could roll the dice in play_once since the value is not needed anywhere else.

get_dice_result():

You could use a list comprehension to save you a bit of typing.

def get_dice_result():
    """Roll the dice five times"""
    return [get_dice_roll() for _ in range(5)]

get_dice_roll():

The random module has a function randint(a, b), which according to the docs is an alias for randrange(a, b+1). Your function could be rewritten to

def get_dice_roll():
    """Roll a dice once"""
    return random.randint(1, 6)

play_once(results):

• There should be no whitespace around keyword arguments (see above).
• The dice can be rolled here. Apart from saving you from passing the array around once, I find that partition more intuitive.
• print("Incorrect. The answer is ", answer, ".", sep = "") can be rewritten as print("Incorrect. The answer is {}.".format(answer)).

get_guess():

except without a specified exception might have some side effects you do not want. An except without a type would also catch KeyboardInterrupt and so on. See this SO answer for further information. Something like

except ValueError:
    # the input could not be parsed as int
    print("\nOops! That is not a number. Try again: ")
except (EOFError, SyntaxError):
    # nothing entered
    print("\nOops! You did not enter anything. Try again: ")

would be better.

get_answer(dice):

There is a semicolon in the return statement.

print_dice(dice):

The print statement can be rewritten as

print('{rows[0]:}\n{rows[1]:}\n{rows[2]:}'.format(rows=rows))

but yours is shorter and clearer. Do not forgot to remove the whitespace around the keyword argument if you keep your code.

main():

A little docstring would be nice.

Final note

As a final result I would say your code is quite good to read. Adding a few docstrings to your functions would further improve the ease of review. I personally would recommend to have a look at PyLint (http://www.pylint.org/), which can do most of the style checking (yes, the ; too) automatically. I use it for all of my projects and it makes writing clean code a little bit easier. Additionally, it will perform some static code analysis.

I hope my feedback will be useful to you and will keep your fun in coding.

Answer 3

The outline of the program is generally fine, so I'll proceed function by function.

display_help

For readability, use a """triple-quoted string""" in conjunction with format(). Note that this means that print_dice() should be changed to return a string instead.

def display_help():
    example = roll_dice()
    print("""The name of the game is Pythons Around the Rose.
The name is important. I will roll five dice,
and I will tell you how many pythons there are.

For example:
{dice}
Will result in {answer}.

If you answer correctly 8 times in a row, you
will be declared a "Pytentate of the Rose".""".format(
        dice=format_dice(example), answer=get_answer(example)
    ))

play

Use a proper while condition.

def play():
    streak = 0
    while streak < 8:
        if play_once():
            streak += 1
        else:
            streak = 0
    print('You are now declared a "Pytentate of the Rose"!')
    print("Thank you for playing!")

get_dice_result and get_dice_roll

The names are a bit off. You aren't fetching anything; you're generating a new result. Also, note that "dice" is the plural form of "die".

You can just combine the two functions into one.

def roll_dice(num_dice=5):
    return [1 + random.randrange(6) for _ in range(num_dice)]

play_once

It's weird that play_once() accepts results as a parameter. I would expect the die roll to be part of this function.

correct_answer would be a more precise name than answer, since "answer" could also refer to the user's input.

def play_once():
    dice = roll_dice()
    print("How many pythons here?")
    print(format_dice(dice))
    correct_answer = get_answer(dice)
    if get_guess() == correct_answer:
        print("Correct!")
        return True
    print("Incorrect. The answer is {}.".format(correct_answer))
    return False

get_guess

Catching all exceptions is a bad idea, as the exception handler could be called for more than its intended purpose. In this case, it also catches EOFError and KeyboardInterrupt, making it impossible for the user to choose to exit the game. (If you want to handle EOFError and KeyboardInterrupt, I suggest putting those handlers in main().)

The guess and valid variables are pointless.

def get_guess():
    while True:
        try:
            return int(input("> "))
        except TypeError:
            print("\nOops! That is not a number. Try again: ")

get_answer

This is more succinct, but it's a matter of taste.

def get_answer(dice):
    return sum(2 if n == 3 else
               4 if n == 5 else
               0
               for n in dice)

print_dice

I like that your source code looks like the die face images. The repeated 0, 1, 2 are a bit tedious. Here is another way you could do it, for your consideration.

def format_dice(dice):
    FACES = [None, [
        "       |",
        "   .   |",
        "       |",
        ], [
        " .     |",
        "       |",
        "     . |",
        ], [
        " .     |",
        "   .   |",
        "     . |",
        ], [
        " .   . |",
        "       |",
        " .   . |",
        ], [
        " .   . |",
        "   .   |",
        " .   . |",
        ], [
        " .   . |",
        " .   . |",
        " .   . |",
        ]
    ]
    return "\n".join(
        ''.join(row) for row in zip('|||', *(FACES[n] for n in dice))
    )