23
\$\begingroup\$

I wrote this code as my first self-directed effort and would appreciate any input on things that I've done seriously wrong. I worry that perhaps I'm over-relying on if-statements, for example, but am not sure how to change the pieces of code to use them less.

import random 

def playchosengame(chosengame):
    if chosengame == 1:
        doorchoice = int(raw_input("How many doors would you like to play with? > "))
        onegameatatime(doorchoice)
    elif chosengame == 2:
        numberofgames = int(raw_input("How many games would you like to play? > "))
        strategy = int(raw_input("Would you like to always change your answer (1) or keep it the same (2)? > "))
        doorchoice = int(raw_input("How many doors would you like to play with? > "))
        onestrategy(numberofgames,strategy,doorchoice)
    else:
        print "What did you say?"


def onestrategy(chosennumberofgames,chosenstrategy,doorchoice):
    # for playing x number of Monty Hall games with one type of strategy
    wincount = 0
    i = 1

    for i in range(1,chosennumberofgames + 1):
        possibleanswers = range(1,doorchoice + 1)
        correctanswer = random.randint(1,doorchoice)
        incorrectpossibilities = possibleanswers
        incorrectpossibilities.remove(correctanswer)

        youranswer = random.randint(1,doorchoice)

        if youranswer == correctanswer:
            otherremaining = random.choice(incorrectpossibilities)
        else:
            otherremaining = youranswer

        incorrectpossibilities.remove(otherremaining)

        print "The correct answer is NOT one of these: %r" % incorrectpossibilities

        if youranswer == correctanswer:
            print "Which means it is either your answer: %r, or the only other remaining option, %r." % (correctanswer, otherremaining)
            if chosenstrategy == 1:
                finalanswer = otherremaining
            else:
                finalanswer = correctanswer
        else:
            print "Which means it is either your answer: %r, or the only other remaining option, %r." % (youranswer, correctanswer)
            if chosenstrategy == 1:
                finalanswer = correctanswer
            else:
                finalanswer = youranswer

        print "You chose %r" % finalanswer

        if finalanswer == correctanswer:
            wincount += 1
            print "You win!"
        else:
            print "You lose!"

        i += 1

    print "You won %r out of %r games. Congrats!" % (wincount, i - 1)

def onegameatatime(doorchoice):
    # for playing one Monty Hall game at a time.
    playagain = 1

    while playagain == 1:
        possibleanswers = range(1,doorchoice + 1)
        correctanswer = random.randint(1,doorchoice)
        incorrectpossibilities = possibleanswers
        incorrectpossibilities.remove(correctanswer)

        youranswer = int(raw_input("Pick a door number from 1 - %r > " % doorchoice))

        if youranswer == correctanswer:
            otherremaining = random.choice(incorrectpossibilities)
        else:
            otherremaining = youranswer

        incorrectpossibilities.remove(otherremaining)

        print "The correct answer is NOT one of these: %r" % incorrectpossibilities

        if youranswer == correctanswer:
            print "Which means it is either your answer: %r, or the only other remaining option, %r." % (correctanswer, otherremaining)
        else:
            print "Which means it is either your answer: %r, or the only other remaining option, %r." % (youranswer, correctanswer)

        finalanswer = int(raw_input("Which do you choose? > "))

        if finalanswer == correctanswer:
            print "You win!"
        else:
            print "You lose!"

        playagain = int(raw_input("Play again? (1 = yes, 0 = no) > "))

    print "Thanks for playing!"

gamechoice = int(raw_input("Play one game at a time (1) or play your chosen number of games with one strategy (2)? > "))
playchosengame(gamechoice)
\$\endgroup\$
1
  • 11
    \$\begingroup\$ Youknowitsprettyhardtoreadlotsofwordswithoutsomedelimiter. Maybethatsworththinkingabout. Underscores_are_the_standard_IIRC. \$\endgroup\$
    – anon
    Commented Dec 17, 2015 at 20:30

4 Answers 4

17
\$\begingroup\$

Welcome to Code Review and coding in general:

  • Code according to PEP8 – In your case this means using snake_case for variable and function names, add spaces after commas and between operators. Your code does look rather nice in general, some minor nitpicks here and there, but read through the guidelines and try to adhere to them.

  • Feature: incorrect_possibilities = possible_answers invalidates possible_answers – When you later on do incorrect_possiblities.remove(correct_answer) this also changes possible_answers. To get a copy you need either to use a better variant of copy, or simply incorrect_possibilities = possible_answers[:]. The latter one uses slicing to denote the entire array, and makes a copy of the sliced area.

  • Use ternary related to chosen_strategy - Based on the chosen strategy you choose either of two choices, this can be coded as final_answer = correct_answer if chosen_strategy == 1 else your_answer, and similar in the other cases.

  • Use ternary related to print output also – This can also be used in print out as well:

    print("Which means it is either your answer: {}, or the only other remaining option, {}.".format(
          correct_answer if your_answer == correct_answer else  your_answer,
          other_remaining if your_answer == correct_answer else correct_answer))
    
  • Use the newer print('You chose {}'.format(final_answer) syntax – This is the preferred version for new code in Python 2 and for code in Python 3. See Format specification mini language for examples and specification.

  • Use docstrings, """ ... """, and not comments, # ..., when describing modules, functions, classes and methods. – If you use docstrings they can be used by your IDE or documentation tools, and it is according to the Pythonic way. :-)
  • Avoid one-time temporary variables – The playagain can be dropped if you use while True: to keep the game going, and change the end to something like if int(raw_input("Play again (0 = no)?")) == 0: break. (Preferrably with a newline before the break, which does break out of the while loop)
  • Possibly add a loop or validation on the main loop – If you enter something when choosing between playing one game at a time, or play a chosen number of games, you can enter something wrong and the script terminates. This could possibly be enhanced by adding a while loop around it.

    Do also note that if you enter non-numeric text your code breaks very easily... Look into making a general int input function which uses try ... except around the input function.

  • Introducing the if __name__ == '__main__': idiom – The previous code is often used to make the code reusable as a module, and to have as little code as possible at the top level. Combined with a main() function your code easily be reused and extended.

    In your case you could use (possibly extended with the while loop):

    def main(): 
        game_choice = int(raw_input("Play one game at a time (1) or play your chosen number of games with one strategy (2)? > "))
        play_chosen_game(game_choice)
    
    if __name__ == '__main__':
        break
    
  • Consider shortening the text length – Your texts are rather worthy, and could benefit from being shortened a little.

Conclusion

Your code does look good for a beginner, and naming is good (with the exception of not using underscores in between words). You could benefit from learning to use the ternary operator (a if b else c), and a slight justification on using functions related to top level code and text lengths.

\$\endgroup\$
10
\$\begingroup\$

Read the BibleStyle Guide

Check out PEP-8, the style guide for Python. The thing that quickly jumps out at me from your code is how many of your variables and functions are lotsofwordsstucktogetherwithoutanyspacing. Those are very difficult to read. The preferred Python syntax is snake_case. That is:

def play_chosen_game(game_choice):
    ...

Much easier to parse.

Python isn't like C/Java

You don't have to predeclare variables, and you don't need to manually increment. Your main loop in one_strategy is:

i = 1
for i in range(1, num_games + 1):
    ...
    i += 1

This is wrong for several reasons: (1) you don't need the initial i = 1 (2) The ending i += 1 is meaningless (3) range() is the wrong construct (4) you reference i at the end of the loop when you really want num_games. Prefer the following:

for _ in xrange(num_games):
    ...

The _ naming makes it clear that the variable isn't important - you're just doing something num_games times. Also, it doesn't matter if we iterate over 1..9 or 0..8, so pick the one that's easier to spell.

Building on this, all we want is to count the number of wins, so let's make that explicit:

wins = 0
for _ in xrange(num_games):
    wins += play_game(num_doors, strategy_choice)

Moving all the logic you had in the loop into its own function.

Picking a door

The logic for picking a door is a little hard to follow. Let's simplify it. We start with all of our doors:

doors = range(num_doors)

We randomly pick one that's correct, and we randomly pick one for our choice:

answer = random.randint(0, num_doors - 1)
first_choice = random.randint(0, num_doors - 1)

Next, based on the strategy, we either stay or pick a different door:

if strategy_choice == 1:
    # pick a new door that isn't first_choice or some random wrong one
    goat = random.choice(
        [i for i in xrange(num_doors) if i not in (first_choice, answer)]
        )
    final_answer = random.choice(
        [i for i in xrange(num_doors) if i not in (first_choice, goat)]
        )
else:
    # stay put
    final_choice = first_choice

Much more direct.

\$\endgroup\$
7
\$\begingroup\$

For your second week of coding, this is good work!

I would recommend reading over Python's official style guide, PEP8.

I would also recommend reading some of our highest rated tagged questions for inspiration into how to improve / things to make.


Space after commas:

In a few instances you're missing space after commas:

for i in range(1,chosennumberofgames + 1):

PEP8 advises to provide a space after commas.


snake_case:

You should be naming your variables in snake_case, which is what PEP8 also advises.

incorrectpossibilities = possibleanswers

Duplicate logic:

I'm seeing a lot of duplicate logic, like a lot:

    if youranswer == correctanswer:
        print "Which means it is either your answer: %r, or the only other remaining option, %r." % (correctanswer, otherremaining)
    else:
        print "Which means it is either your answer: %r, or the only other remaining option, %r." % (youranswer, correctanswer)

I suggest simplifying these by use of different methods:

    answer_to_print_one = your_answer
    answer_to_print_two = correct_answer
    if your_answer == correct_answer:
        answer_to_print_one = correct_answer
        answer_to_print_two = other_remaining
    print "Which means it is either your answer: {a1}, or the only other remaining option, {a2}.".format(a1=answer_to_print_one, a2=answer_to_print_two)

Using "" %:

This is an outdated and inefficient way of string concatenation:

Use "{string}".format(string=your_string) style concatenation.


Using a class:

At the moment, you have a bunch of random, relational functions that pass input around, parameters etc.

Use a class instead, this allows you to:

  • Link the functions properly
  • Store parameters at a class level, reducing need for parameters
  • Store multiple instances of games
\$\endgroup\$
4
\$\begingroup\$

Your names are clear and that's good, but some of your names have a redundancy issue particularly from being verbose.

For example:

def playchosengame(chosengame):

You realise this is just as readable like this:

def play(chosengame):

And of course you just call it with

play(gamechoice)

If there was another function that played music, then you may need to add play_game, but as it stands there's little ambiguity because of how the parameters are being named. It's more important that the lines read well, rather than trying to make each name tell you everything about its value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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