3
\$\begingroup\$

This game allows one to either pick their own lottery numbers or have them generated and checks them with the most recent numbers, jackpot, and multiplier. I use concepts such as random, for loops, raw_input, and logic. I would it appreciated if it were reviewed by someone more experienced.

import random
choice = raw_input("Do you want to randomly generate numbers, or pick them? (Enter 'R' for random, 'P' for pick.")
if choice == ('R'):
  one = random.randint(1,69)
  two = random.randint(1,69)
  three = random.randint(1,69)
  four = random.randint(1,69)
  five = random.randint(1,69)
  pball = random.randint(1,26)
elif choice == ('P'):
  one = int(raw_input("Please pick your 1st number, between 1 and 69."))
  two = int(raw_input("Please pick your 2nd number, between 1 and 69."))
  three = int(raw_input("Please pick your 3rd number, between 1 and 69."))
  four = int(raw_input("Please pick your 4th number, between 1 and 69."))
  five = int(raw_input("Please pick your 5th number, between 1 and 69."))
  pball = int(raw_input("Please pick your Powerball number, between 1 and 26."))
  if ((one or two or three or four or five) > 69) or pball > 26 or ((one or two or three or four or five or pball) < 1): #In case the entered numbers are too high or too low.
    print "Please choose a number between 1 and 69."
elif choice != ('R' or 'P'): #In case invalid input is given.
  print "Please enter either 'R' for random or 'P' for picking."
human = [one, two, three, four, five, pball]
print human
#August 15, 2018 Powerball
real = [12,15,28,47,48,16]
jackpot = 40000000
multiplier = 2
def powerball(human,real,jackpot,multiplier):
  x = raw_input("Do you want to buy the multiplier for $1? ('Y' or 'N')")
  if x == 'Y':
    pass
  elif x == 'N':
    multiplier = 1
  p = 0 #Prize
  i = 0 #Index
  m = 0 #Matches
  if human[5] == real[5]: #If you only get the powerball right.
    p = 4
  for i in range(0,5): #If you only get normal numbers, no powerball.
    if human[i] in real:
      m += 1
    i += 1
    if m == 3:
      p = 7
    elif m == 4:
      p = 100
    elif m == 5:
      p = 1000000
  m = 0
  for i in range(0,5): #If you get normal numbers along with the powerball.
    if (human[i] in real) and (human[5] == real[5]):
      m += 1
    i += 1
    if m == 1:
      p = 4
    elif m == 2:
      p = 7
    elif m == 3:
      p = 100
    elif m == 4:
      p = 50000
    elif m == 5:
      p = jackpot
  p *= multiplier
  if p > 0:
    return "Congratulations, you won $%s!" % (p)
  elif p == 0:
    return "Sorry, you didn't win anything, try again next time."
print powerball(human, real, jackpot, multiplier)
\$\endgroup\$
2
\$\begingroup\$
  • Conditions

    if ((one or two or three or four or five) > 69)
    

    one or two will return one if it isn't falsy (0 / None / False), you should use separate conditions, like so :

    if (one > 69 or two > 69 or three > 69...)
    

    alternatively, you could just check the maximum value of your list

    if (max(one, two, three, four, five) > 69)
    

    The same problem occur in this part :

    elif choice != ('R' or 'P')
    

    it should be elif choice not in ('R','P')

  • variable names

    instead of writing

    m=0#matches
    

    directly use the variable matches, your code will be easier to understand

  • loops

    for i in range(0,5): #If you only get normal numbers, no powerball.
        if human[i] in real:
          m += 1
        i += 1
    

    you don't need to increase i in a for loop, it is done automatically

  • code structure

    in the powerball function, you have the following blocks:

    if powerball:
        price = 4
    
    for i in range(0,5):#no powerball
    ...
    for i in range(0,5):#yes powerball
        if powerball and human[i] == real:
            ...
    

    You should change that in a if/else structure with the if powerball while counting the number of corresponding number outside :

    matches = 0
    price = 0
    
    for i in range(0,5):
        if human[i] in real:
            matches+=1
    
    if human[5] == real[5]:# with powerball
        if matches == 0:
            price = 4
        elif matches == 1:
            price = 4
    else:# without powerball
        if matches == 3:
            price = 7
        elif matches == 4:
            price = 100
    
  • multiple elif for variable assignation

    (I don't know if this one is considered good practice, but it's faster to use and to add case) instead of writing multiple elif, try to use a dictionary :

    prices_for_matches = { 3 : 7,
                           4 : 100,
                           5 : 1000000}
    if matches in prices_for_matches:
        price = prices_for_matches[matches]
    else:
        price = 0
    

note i don't know much about powerball, but your solution doesn't care about the order of the balls (human[i] in real instead of human[i] == real[i]) or if the user selected multiple times the same number

\$\endgroup\$
1
\$\begingroup\$

this is a good start, there are a couple of bugs and a couple of places where we can make the code more concise. In order to make the code simpler (and thus less likely to have bugs) we will look at snippets of code near each other and check how similar they are. Anywhere we find blindingly obvious repeats, we will try and make a function and make it do the work.

I won't make too many comments on style or formatting, if you want to find out more yourself a good place to start is pep8 or run your code through a linter and start googling the lint warnings. Some of the stuff to look at would be unnecessary brackets and comments that don't add much value. I'll do my best to make any code I produce here follow best practices.


All the code at the start deals with asking the user for input and validating the input.

if choice == ('R'):
  one = random.randint(1,69)
  two = random.randint(1,69)
  three = random.randint(1,69)
  four = random.randint(1,69)
  five = random.randint(1,69)
  pball = random.randint(1,26)

There is nothing special about one, two, three, four, or five. pball might be dealt with differently later, but it isn't all that special either. So since we never need to distinguish them, lets put them into a list.

if choice == "R":
    balls = [None, None, None, None, None, None]
    balls[0] = random.randint(1,69)
    balls[1] = random.randint(1,69)
    balls[2] = random.randint(1,69)
    balls[3] = random.randint(1,69)
    balls[4] = random.randint(1,69)
    balls[5] = random.randint(1,26)  # The powerball is in the last slot.

This seems fairly repetitious, we can instead use list syntax.

if choice == 'R':
    # The powerball is in the last slot.
    balls = [random.randint(1,69), random.randint(1,69), random.randint(1,69), random.randint(1,69), random.randint(1,69), random.randint(1,26)]

This is still really repetitive (and actually a bit too long too), we can use a list comprehension to save some typing.

if choice == "R":
    balls = [random.randint(1,69) for _ in range(5)]
    powerball = random.randint(1, 26)

elif choice == ('P'):
  one = int(raw_input("Please pick your 1st number, between 1 and 69."))
  two = int(raw_input("Please pick your 2nd number, between 1 and 69."))
  three = int(raw_input("Please pick your 3rd number, between 1 and 69."))
  four = int(raw_input("Please pick your 4th number, between 1 and 69."))
  five = int(raw_input("Please pick your 5th number, between 1 and 69."))
  pball = int(raw_input("Please pick your Powerball number, between 1 and 26."))

We have the same opportunity here to reduce the amount of code. This is a little more finicky because of the cardinal counting (1st, 2nd, etc) but I think it would be worth doing.

elif choice == 'P':
    message = "Please pick your {} number, between 1 and 69 (inclusive)."
    balls = []
    for s in ("1st", "2nd", "3rd", "4th", "5th"):
        ball = int(raw_input(message.format(s)))
        balls.append(ball)
    powerball = int(raw_input("Please pick your powerball number, between 1 and 26 (inclusive)."))

The second last line is a little too long. Another issue we haven't fixed is handling users who don't enter valid numbers. See if you can come up with a way to fix these problems. For the latter one try-except is the way to go.


if ((one or two or three or four or five) > 69) or pball > 26 or ((one or two or three or four or five or pball) < 1): #In case the entered numbers are too high or too low.
  print "Please choose a number between 1 and 69."

This doesn't actually validate numbers correctly. Let's break it down the way python would read it.

(one or two or three or four or five)

This will check each number is "truthy". For ints, this is checking if the number is nonzero. So if any of the numbers are not zero (one or two or three or four or five) becomes True.

((one or two or three or four or five) > 69)  # This becomes
(True > 69)  # True is NOT greater than 69
False

So we need to fix this. Latter in the statement

... or ((one or two or three or four or five or pball) < 1)
(True < 1)  # Assuming at least one of the numbers is nonzero.
False  # Maybe surprisingly, True == 1.

So if we look at the overall if statement

if ((one or two or three or four or five) > 69) or pball > 26 or ((one or two or three or four or five or pball) < 1):
if False or pball > 26 or False:
if pball > 26:  # False or X is the same as X.

So we need to check every number individually. We can write this very cleanly using the built in function 'any'

if any(ball > 69 for ball in balls) or powerball > 26 or any(ball < 1 for ball in balls):

This highlights two things, we never check if the powerball is less than 1, and we keep using these "magic" numbers. We should add that check, and make some variables for these numbers.

# Somewhere near the top of the code
MIN_BALL_VALUE, MAX_BALL_VALUE = 1, 69
MIN_POWERBALL_VALUE, MAX_POWERBALL_VALUE = 1, 26

I'll change any magic numbers to use these variable from now on.

So the validation check would look something like this:

if any(ball < MIN_BALL_VALUE or ball > MAX_BALL_VALUE for ball in balls) or\
   powerball < MIN_POWERBALL_VALUE or powerball > MAX_POWERBALL_VALUE:

elif choice != ('R' or 'P'): #In case invalid input is given.

One small change, this can just be an else statement. It is easy to get (boolean) logic wrong so if you ever get the chance to write something as simple as "else", take it.


def powerball(human,real,jackpot,multiplier):

What do these arguments do? Spend 3 minutes writing a (short) docstring and other people who read your code will thank you. The supplied ones are not the best, hopefully it shows you some things you can add to the

def powerball(human,real,jackpot,multiplier):
    """Check if the balls passed won the powerball. Return a message with the result.
        human: List[Int], the user's choice of balls. Assumes the last ball is the powerball.
        real: List[Int], the winning balls.
        jackpot: Int, the best prize possible.
        multiplier: Int, if a prize is won, multiplies it.

        Examples:
            powerball([1, 2, 3, 4, 5, 6], [1, 2, 3, 4, 5, 6], 10, 2) ->'Won the jackpot of 10 * 2 == 20!'
            powerball([1, 2, 3, 4, 5, 6], [11, 12, 13, 14, 15, 16], 10, 2) -> 'You get nothing!'"""
    ...

x = raw_input("Do you want to buy the multiplier for $1? ('Y' or 'N')")
if x == 'Y':
  pass
elif x == 'N':
  multiplier = 1

This is asking the user for input, and changing the multiplier based on it. It is rather unexpected to have this inside this function and away from the other user input. I would suggest moving it up to the other code that deals with user input.


p = 0 #Prize
i = 0 #Index
m = 0 #Matches

Rather than having a single word comment with what the variable is, just use the word. They are great names, and it is free to use more characters in variable names.

prize = 0
index = 0
matches = 0

if human[5] == real[5]: #If you only get the powerball right.
  p = 4
for i in range(0,5): #If you only get normal numbers, no powerball.
  ...
for i in range(0,5): #If you get normal numbers along with the powerball.
  ...

The code here looks a little strange. I would avoid changing the iterating variable (i) inside a loop. We also have some repeated code to reduce. I won't delve into this code (apologies!) as I don't think I'd correctly root out all the bugs.Instead I will offer an alternative implementation, which should hopefully highlight some test cases to watch out for.

We want to count the number of unique matches. There are some annoying cases like human = [1, 1, 1, 1, 1, 1], real = [1, 2, 3, 4, 5, 1] which shouldn't be allowed. So we are going to be defensive and check everything. To make life easier, take the powerball out of both sets of balls.

balls, powerball = human[:-1], human[-1]
# In python3 you could write *balls, powerball = human
if len(set(balls)) != len(balls):
    raise ValueError("The balls have duplicates!")

winning_balls, winning_powerball = real[:-1], real[-1]
if len(set(winning_balls)) != len(winning_balls):
    raise ValueError("The winning balls have duplicates!")

matches = sum(ball in winning_balls for ball in balls)
matched_powerball = powerball == winning_powerball

We can count the matches by a nice sum. We get 1 if a ball is in the set of winning balls and 0 otherwise. So summing them gives the total number of matches.

prizes = {
    # (matches, matched_powerball): prize
    (3, False): 7,
    (4, False): 100,
    (5, False): 1000000,
    (1, True): 4,
    (2, True): 7,
    (3, True): 100,
    (4, True): 50000,
    (5, True): jackpot}

if (matches, matched_powerball) in prizes:
    prize = prizes[(matches, matched_powerball)]
elif matched_power_ball:
    prize = 4

p *= multiplier
if p:
    return "Congrats, ..."
return "Sorry, ..."

A couple of final points, the prizes dictionary is mostly static and could be moved outside of the function. That's a good candidate for either a global variable or some other method of configuration management. Notice how the final lines no longer have an if-elif statement, just if. This is the same reasoning as above, if you can rewrite your code with less elifs, it means there are less chances to introduce bugs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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