2
\$\begingroup\$

I've been working on Python for the past few weeks trying to get my head around it's syntax/naming conventions/style/etc. I decided to make a monty hall sim to see how it looks in python. For those who don't know what the Monty Hall problem is, check the second paragraph here.

I've verified the code is fully functional and gives the right results. It IS indeed best to swap every time. After spending time getting rid of duplicated effort, overly wordy statements, and many logic errors, I'd like to know how to improve my code readability and efficiency (both here and in general).

Specific to this code, here's what I'd like to know (mostly #1):

  1. If possible, how could I make this more efficient?

  2. Is having a function call in the first for loop a good idea? If not, where do I put it?

  3. I keep feeling like this is longer than needed. Are there still places I can cut out code?

import random

def setup():
    # Basic greeting/explaining what to do.
    print("\n\nThere are 3 doors. One is a winner. The computer")
    print("will try guess the right one. After the first ")
    print("guess, one door is shown to be a loser. The computer ")
    print("can then change it's choice to the remaining door.\n")

    # Display choices and get input
    change_choice = input("Should the computer always change it's choice? y/n\n:> ")
    runs = int(input("How many times should this run?\n:> "))
    run(change_choice, runs)

def run(change_choice, times_to_run):
    wins = 0 #Correct door choices

    # Loop through calling the function with switching enabled or not   
    for i in range(times_to_run):
        if change_choice == 'y' or change_choice == 'Y':
            wins = wins + setupDoors(True)
        else:
            wins = wins + setupDoors(False)

    # Display amount of times won/percentage of games won
    statistics(wins, times_to_run)

def setupDoors(always_change_choice):
    # Set up the winning door
    winning_door = random.randint(1,3)
    print("\nThe winning door is "+str(winning_door))

    # Get the computer's guessed door
    computer_guess = random.randint(1,3)
    print("the computer guessed door number "+str(computer_guess))
    return playGame(winning_door, computer_guess, always_change_choice)

def playGame(winning_door, computer_guess, always_change_choice ):

    # Get the right door to open.
    # Can't be the computer's door OR the winning door
    opened_door = getAvailableDoor(computer_guess, winning_door)
    print("Door number "+str(opened_door)+" was opened and shown not to be a winning door.")

    #Swap the computer's choice to last door if that was desired
    if always_change_choice:
        computer_guess = getAvailableDoor(computer_guess, opened_door)
        print("The computer changed it's guess to door number "+str(computer_guess))

    # Return a win or a loss.
    if computer_guess == winning_door:
        print("The computer won!")
        return 1
    else:
        print("The computer lost!")
        return 0

def getAvailableDoor(computer_guess, unavailable_door):
    # List of doors to choose from
    Doors = [1,2,3]

    for i in range(3):
        if Doors[i] == computer_guess or Doors[i] == unavailable_door:
            continue
        else:
            break
    return Doors[i]

def statistics(wins, times_to_run):
    print("\nThe computer won "+str(wins)+" times")
    print("This means the computer won "+str((wins/times_to_run)*100)+"% of the time")

setup()
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Readability:

  • you should have 2 newlines after each function definition
  • you should use augumented assignments where possible
  • you should define each method in a logical order (e.g: if you're calling a function inside another function, the later should be defined first). That's rather a personal preference because you can't do this everytime.
  • you should have a space around operators and delimiters
  • you should make your print() statements more compact
  • variable names should be snake_cased. The same goes for method names.
  • at the beginning of each function you should define a docstring

Now, let's dig into each function:

  • in def statistics() you should use format() and join those two prints into a single one:
def statistics(wins, times_to_run):
    """Print the statistics"""
    print("The computer won {} times"
          "This means the computer won {} % of the time".format(wins, (wins / times_to_run) * 100))
  • as mentioned above:
def setup_doors(always_change_choice):
    """Set up the winning door"""
    winning_door = random.randint(1, 3)
    computer_guess = random.randint(1, 3)

    print("The winning door is {}.\n"
          "The computer guessed door number {}".format(winning_door, computer_guess))

    return play_game(winning_door, computer_guess, always_change_choice)
  • in def run(change_choice, times_to_run) you should avoid that multiple condition by using the lower function. You can also use the enumerate() to get rid off that variable definition, and replace i by _ as you don't use it:
def run(change_choice, times_to_run):
    """Some description here"""
    for wins, _ in enumerate(range(times_to_run)):
        if change_choice.lower() == 'y':
            wins += setup_doors(True)
        else:
            wins += setup_doors(False)

    # Display amount of times won/percentage of games won
    statistics(wins, times_to_run)
  • in def setup() I'd just join the print statements:
def setup():
    """Basic greeting/explaining what to do."""
    print("\n\n"
          "There are 3 doors. One is a winner. The computer \n"
          "will try guess the right one. After the first \n"
          "guess, one door is shown to be a loser. The computer \n"
          "can then change it's choice to the remaining door.\n")

    # Display choices and get input
    change_choice = input("Should the computer always change it's choice? y/n\n:> ")
    runs = int(input("How many times should this run?\n:> "))
    run(change_choice, runs)

// To be continued. I have to leave the office.

\$\endgroup\$
4
  • \$\begingroup\$ Even without continuation, this is better and far more feedback than I could have hoped for. Thank you! In regards to augmented assignments, I think I tried something like x++ once and found out that didn't work. Then assumed +=, /=, *=, -=, etc wouldn't work either. Lesson: Don't assume! The only part I'm having trouble following is the modification to the run() function. I do see that I'm not using i, but why is wins right after the for? I think I understand the rest of the line after it. \$\endgroup\$
    – ephreal
    Commented Dec 16, 2016 at 23:00
  • \$\begingroup\$ What do you think of '{:.1%}'.format(.25) vs '{}%'.format(.25*100)? \$\endgroup\$
    – Peilonrayz
    Commented Dec 16, 2016 at 23:28
  • 1
    \$\begingroup\$ Yeah, the run function is confusing. As written now, it would be equivalent to write for wins in range(times_to_run). \$\endgroup\$
    – Graipher
    Commented Dec 17, 2016 at 7:09
  • \$\begingroup\$ That's what I was thinking. It seems weird to remove a variable from a for loop just to add one in. And @Peilonrayz, I did stumble across that while researching the format function on strings. I eventually settled on {:.2f} to give me two decimal places. \$\endgroup\$
    – ephreal
    Commented Dec 17, 2016 at 18:44

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.