Pythons Around the Rose

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.