6
\$\begingroup\$

How could I improve this code or make it more efficient?

import random

menu_check = True

# 'checker', compares userNums and winningNums to see if they have won or lost
def checker(userNums, winningNums):

    if userNums == winningNums:
        print ("\nCongratulations! You Win $100!\n")
        print ("Your numbers: ", userNums)
        print ("The winning lottery numbers are: ", winningNums, "\n")
    else:

        print ("\nSorry, you lose...\n")
        print ("Your numbers: ", userNums)
        print ("The winning lottery numbers are: ", winningNums, "\n")

# 'getUserNums', gets user numbers and puts into a sorted list    
def getUserNums():
    userNums = []

    for x in range(3):
        nums = int(input("Pick a number 0 through 9: "))
        if 0 <= nums <= 9:
            userNums.append(nums)
        else:
            input("Error! Invalid input. Press any key to continue...")
            nums = int(input("Pick a number 0 through 9: "))
            userNums.append(nums)

    return sorted(userNums)

# 'getWinningNums', creates a sorted list with random nums ranging from 0-9 with a range of 3 values
def getWinningNums():
    return sorted(random.sample(range(0,10), 3)) 

# 'menu', creates the main menu to choose game or exit program
def menu():
    print (30 * "-", "LOTTERY MENU", 30 * "-")
    print ("1. [Play Pick-3]")
    print ("2. Exit")
    print (75 * "-")

# 'main', calls the other functions
def main():
    userNums = getUserNums()
    winningNums = getWinningNums() 
    checker(userNums, winningNums)

#         
while menu_check:
    menu()
    choice = input("\nEnter your choice[1-2]: ")

    if choice == '1':
        print (23 * "-")
        print ("[Play Pick-3] selected!")
        print (23 * "-")
        menu_check = False
        main()

    elif choice == '2':
        print ("\nThanks for playing!\n")
        menu_check = False
    else:
        input("Error! Invalid input. Press any key to continue...\n")
\$\endgroup\$
1
  • \$\begingroup\$ I'm not sure how much you assume out of your user, but logic in getUserNums isn't foolproof. For example if I input a letter instead of a number an execption will be thrown because int() expects a number. Also if I enter a number less than zero or greater than 9 TWICE it'll store a number that is outside your intended range. I'm not sure if you wanted that to happen or not. \$\endgroup\$
    – Dzhao
    Jun 23, 2016 at 14:54

2 Answers 2

8
\$\begingroup\$

General comments

For the most part this is very good code imho. You have a clear and consistent style. I really like you have split your functions into logical functions. You are however missing writing a more pythonic style function, and parts of your code is very hard coded.

Style

You have a very consistent coding style which is good. However you seem to use a #C naming convention and formating instead of a pythonic. You are using camelCase for variable names however in Python recommend UpperCamelCase for class names, CAPITALIZED_WITH_UNDERSCORES for constants, and lowercase_separated_by_underscores for other names.

The golden standard for style in python isPEP 8. It explains in excruciating detail how to structure your code. I whole heartily recommend skimming through it and follow it.

Semantics

You should use the if __name__ == "__main__": module in your answer. This allows you to reuse the functions for later, and is a great thing to always use.

The getUserNums() is a bit strange. If my input is incorrect, you raise an error. However the second time my input is incorrect it passes just fine. You could solve this using a while loop instead

def getUserNums():
    userNums = []

    while len(userNums) < 3:
        nums = int(input("Pick a number 0 through 9: "))
        if 0 <= nums <= 9:
            userNums.append(nums)
        else:
            input("Error! Invalid input. Press any key to continue...")

The second problem with the snippet above is that you can guess the same number more than once! A quick fix for this would be to replace if 0 <= nums <= 9 with

if 0 <= nums <= 9 and nums not in UserNums

A third and final problem is that you'r program crashes if the input is not an integer. There are many solutions for this, a quick and dirty one is shown below

    nums = input("Pick a number 0 through 9: ")
    try:
        nums = int(nums)
    except:
        print("Sorry your input must be an integer!")
        continue

The continue keyword is important as it skips the remainder of the loop. What happens if you remove it?

You have repeated uses of print in the menu() function, (a better name would perhaps have been print_menu()). This can be solved as follows

 def menu():
    print (30 * "-", "LOTTERY MENU", 30 * "-",
          '1. [Play Pick-3]",
          '2. Exit',
           75 * "-")

Magic numbers

Throughout your code we find what is called Magic numbers. They are unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants. For example the range of numbers or 30*'-'. These should optimally be replaced with named constants.

In the code below I have changed your variable names, replaced all the magical numbers. I have also made some small changes to your main function.

Lasly I made your Error messages more descriptive: input("Error! Invalid input. Press any key to continue...") Firstly what is the error? What was wrong with my input? Secondly it is strange to ask the user to press any key to continue.

Code

import random

NUMBER_OF_PICKS = 3
MIN_PICK = 1
MAX_PICK = 11
WINNINGS = 100
OFFSETT = 4


# 'checker', compares userNums and winningNums to see if they have won or lost
def checker(userNums, winningNums):
    if userNums == winningNums:
        print ("\nCongratulations! You Win ${}!".format(WINNINGS),
               "\nYour numbers: ", userNums,
               "\nThe winning lottery numbers were: ", winningNums, "\n")
    else:

        print ("\nSorry, you lose...",
               "\nYour numbers: ", userNums,
               "\nThe winning lottery numbers were: ", winningNums, "\n")


# 'get_user_nums', gets user numbers and puts into a sorted list    
def get_user_nums():
    userNums = []
    while len(userNums) < NUMBER_OF_PICKS:
        nums = input("Pick a number {} through {}: ".format(MIN_PICK, MAX_PICK))
        try:
            nums = int(nums)
        except:
            print("Sorry your input must be an integer!")
            continue
        if MIN_PICK <= nums <= MAX_PICK:
            if nums not in userNums:
                userNums.append(nums)
            else:
                print("Error! You have already picked this number")
        else:
            print("Error! Your number was not in range")

    return sorted(userNums)


# 'get_winning_nums', creates a sorted list with random nums ranging from 0-9 with a range of 3 values
def get_winning_nums():
    return sorted(random.sample(range(MIN_PICK, MAX_PICK), NUMBER_OF_PICKS)) 


# 'menu', creates the main menu to choose game or exit program
def lottery_menu():
    name = ' '*int(OFFSETT/2) + "LOTTERY MENU"
    dotted = (OFFSETT+len(name))*'-'
    options = ["[Play Pick {}]".format(NUMBER_OF_PICKS), 
               "[Exit]"]
    print('{} \n{} \n{}'.format(dotted, name, dotted))
    for i, opt in enumerate(options):
        print(i+1, opt)
    print(dotted)


def play_pick_n():
    userNums = get_user_nums()
    winningNums = get_winning_nums() 
    checker(userNums, winningNums)


# 'main', calls the other functions
def main():
    lottery_menu()
    while True:
        choice = input("\nEnter your choice[1-2]: ")
        if choice == '1':
            string = "\n[Play Pick {}]".format(NUMBER_OF_PICKS) + "selected!"
            dotted = '\n'+ len(string) * "-"
            
            print(dotted,
                  string,
                  dotted)
            
            play_pick_n()
            break

        elif choice == '2':
            print ("Thanks for playing!\n")
            break
                         
        print("Error! Invalid input. Press any key to continue...\n")
        
if __name__ == '__main__':
    main()
\$\endgroup\$
2
\$\begingroup\$

A small comment, but you have duplicated code here:

if userNums == winningNums:
    print ("\nCongratulations! You Win $100!\n")
    print ("Your numbers: ", userNums)
    print ("The winning lottery numbers are: ", winningNums, "\n")
else:

    print ("\nSorry, you lose...\n")
    print ("Your numbers: ", userNums)
    print ("The winning lottery numbers are: ", winningNums, "\n")

Since in either case you intend to print the user's numbers and the winning numbers, pull that out:

if userNums == winningNums:
    print ("\nCongratulations! You Win $100!\n")
else:
    print ("\nSorry, you lose...\n")
print ("Your numbers: ", userNums)
print ("The winning lottery numbers are: ", winningNums, "\n")

Now if you decide to change either of those, you change it in only one location.

Additionally, it's great that you're commenting each function - but those comments would be even better if they were docstrings. For example:

# 'checker', compares userNums and winningNums to see if they have won or lost
def checker(userNums, winningNums):

Would become:

def checker(userNums, winningNums):
"""Compares userNums and winningNums to see if they have won or lost
@param userNums: description of userNums
@param winningNums: description of winningNums
"""

A docstring might be overkill for a small project like this, but since you're basically writing them anyways, you might as well put them where people will expect them (and where information on the function will be

\$\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.