Simple addition, multiplication, divison, and subtraction program

Question

I made a post a couple of days ago asking if anyone noticed bad habits or thought of any suggestions: (First Hangman game)

I've made 2 math programs just to make sure I got it all down pat. The first one (the subject of this question) is a simple addition, multiplication, division, and subtraction program that would deal out random numbers and ask you to add, sub, multiply, or divide them.

#12/14/14
import random

def choose():
    num1, num2 = pick_numbers()
    types = input("What type would you like Add, Sub, Mutl, Div?\n> ")
    if types.upper() == "ADD":
        print("Creating your addition problem...")
        print("{} + {}".format(num1, num2))
        correct_answer = num1 + num2
        return correct_answer
    elif types.upper() == "SUB":
        print("Creating your subtraction problem...")
        print("{} - {}".format(num1, num2))
        correct_answer = num1 - num2
        return correct_answer
    elif types.upper() == "MULT":
        print("Creating your mutliplecation problem...")
        print("{} * {}".format(num1, num2))
        correct_answer = num1 * num2
        return correct_answer
    elif types.upper() == "div":
        print("Creating your divison problem...")
        print("{} / {}".format(num1, num2))
        correct_answer = num1 / num2
        return correct_answer
    else:
        print("Error: Please only choose, add, sub, mult, or div")


def pick_numbers():
    num1 = random.randint(1, 100)
    num2 = random.randint(1, 100)
    return num1, num2

def print_problem():
    correct_answer = choose()
    while True:
        answer = input("What is your answer?\n> ")
        if(int(answer) == correct_answer):
            print("Correct! The answer was {}".format(answer))
            return False
        else:
            print("Sorry {} isn't correct.".format(answer))

def play_again():
    try_again = input("Would you like to play again?\n> ")
    if try_again.upper() == "Y":
        return True
    elif try_again.upper() == "N":
        return False
    else:
        print("Error: please choose only 'Y' or 'N'")

while True:
    print_problem()
    if not play_again():
        break

Do you still notice any bad habits or have any suggestions so I can have better structure or code organization?

Answer 1

• Be careful with exceptions. int(answer) may raise a ValueError if an answer cannot be converted. Same goes for unchecked division.

• In case of an illegal choice, choose doesn't return a value. Not a problem here, but rather a bad habit.

• if/elif/elif/... cascade is usually better expressed via a dictionary, in this case of 2-tuples (a format string and a lambda:

  problems = { "ADD": ("{} + {}", lambda x,y: x + y), 
               "MULT": (...),
                etc }
  

  Then the body of choose could be reduced to

  try:
      format, action = problems[types.upper()]
      print format.format(num1, num2)
      return action(num1, num2)
  except KeyError:
      print "Choose only from %s" % ",".join([key for key in problem.keys()])
  

  and you may freely add more operations to problems not ever touching the code.

Answer 2

Just to point out a bug in your text processing. You convert all the operations to 'upper', but for division, you compare it to the lowercase value ... == "div". This will never match.

Your testing has not identified this problem, and it is a good reminder to have comprehensive tests, and to be careful with copy/paste.

Finally, the repeated use of types.upper() indicates that it should be stored in a variable, and reused.

Answer 3

A few minor points on top of the other reviews.

In print_problem, when the answer is correct, you return True, but the return value of this method is never used. It would be better to simply return (without arguments), or even just to simply break to get out of the loop and let the method end normally.

To avoid the repetition of .upper() calls in here:

if types.upper() == "ADD":
    # ...
elif types.upper() == "SUB":
    # ...
elif types.upper() == "MULT":
    # ...

You could have done this:

types = input("What type would you like Add, Sub, Mutl, Div?\n> ").upper()

Actually, typing out ADD, SUB, ... is a bit harder than add, sub, ..., so instead of converting the input to upper and comparing with uppercase constants, I'd convert the input to lower and compare with lowercase constants.

In play_again, the last else statement is unnecessary, because the if and elif branches always return. In fact you could also replace the elif with an if for the same reason. Applying similar suggestions related to case conversion, the implementation becomes slightly simpler:

try_again = input("Would you like to play again?\n> ").lower()
if try_again == "y":
    return True
if try_again == "n":
    return False
print("Error: please enter 'Y' or 'N'")