Transform Python math game to OOP

Question

Background: I'm trying to learn basic OOP with Python. I have this program I wrote to help my son practice math problems. It does addition, subtraction, multiplication, and division. He can choose easy, medium, or hard for each operator.

Here's an excerpt of the code. It's the "easy" addition function. (The "medium" and "hard" functions simply choose bigger random numbers.)

def addition_problems_1():
    global name
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)
        sum = a + b

        answer = int(raw_input("%i + %i = " % (a, b)))

        if answer == sum:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != sum:
            print "Oops, the correct answer is %i. Try another one." % sum

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And here's the "easy" Subtraction function. (The multiplication and division functions are very similar.)

def subtraction_problems_1():
    global name
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)

        if a > b:
            sum = a - b
            answer = int(raw_input("%i - %i = " % (a, b)))
        else:
            sum = b - a
            answer = int(raw_input("%i - %i = " % (b, a)))

    if answer == sum:
        score = score + 1
        print "Good job. Current score is %d" % score

    elif answer != sum:
        print "Oops, the correct answer is %i. Try another one." % sum

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

My Question: Could this script be shortened and/or simplified by re-writing it in OOP style? If so, can you show me an example of what that might look like?

Answer 1

It is rather difficult to provide a complete review of the code without having access to it as a whole. For instance, the global name line in both functions is rather bad. Firstly because you're not modifying but just accessing name's value so you don't need it. Secondly because using this kind of global variables is bad anyway, and this might have different ways of solving depending on what you're doing with name in other parts of your code (to which OOP might be a way, but not necessarily the way).

Abstract the problem

Do not repeat yourself, it is bad practice and impair maintainability. Instead try to extract a pattern and parametrize it with the different values you need. Not accounting for the if a > b test in the substraction for now, the only thing that differ between the two functions is:

1. The operation to perform (both taking two arguments);
2. The representation of this operation.

You can thus define

def problems_1(operation, symbol):
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)
        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And call it using one of three ways:

def add(a,b):
    return a + b
def sub(a,b):
    return a - b
...
problems_1(add, '+')
problems_1(sub, '-')

problems_1(lambda a,b: a+b, '+')
problems_1(lambda a,b: a-b, '-')

import operator

problems_1(operator.add, '+')
problems_2(operator.sub, '-')

Using the operator module is neater since it allows you to reuse existing code.

Account for sorting operands

We left aside the problem of sorting the operands for the substraction problem. This can be incorporated back using a third parameter to tell the function whether or not we should try to sort the operands:

def problems_1(operation, symbol, sort=False):
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

You can then call your function using

import operator

problems_1(operator.add, '+')
problems_1(operator.sub, '-', True)

You can also choose to sort unconditionally, it won't change the expected output for + or *; and it will force results to be greater than 1 for / (asuming your “divisions” functions ask for integral division). But having this optional parameter can come in handy when dealing with more difficult problems that can potentially benefit from having substraction produce negative results.

Expand on greater difficulty

I guess that the _1 at the end of the function name is for “easy” and that you have _2 and _3 types of functions which all look the same.

Again, don't repeat yourself¹. Parametrize your function instead. If all you need to change is the range of the randint calls, then you should use:

def problems(operation, symbol, min_op, max_op, sort=False):
    score = 0

    while score < 30:
        a = randint(min_op, max_op)
        b = randint(min_op, max_op)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And that's it. This single tiny change allows you to remove at least 8, all look-alike, functions.

The call then becomes:

import operator

problems(operator.sub, '-', 1, 10, True) # easy
problems(operator.sub, '-', 10, 100, True) # medium
problems(operator.sub, '-', 100, 1000) # hard

Coding standards

Now that we removed a whole bunch of your code at once, let's write it properly. For starter, you should have noticed that I changed your sum variable into result. This is both because it better express the intent and because sum is a builtin function in Python that you are shadowing. Try to avoid using builtin functions names for your variables, it makes the code less understandable.

Second, the use of % format specifiers is to be replaced with the format string syntax. It is pretty simple for basic use (you don't even need to handle types yourself) and can be much more expressive it need be. For instance "%i %s %i = " % (a, symbol, b) becomes "{} {} {}".format(a, symbol, b).

Next, answer == sum and answer != sum are already complementary clauses. There is no third (or fourth, or whatever) choice. Thus you can use an else instead of your elif.

Last, your code will fail if the user enters something that fails to be an integer. int(raw_input(...)) will raise ValueError in such cases. You could account for that using a try .. except clause and providing a default value to answer (such as None) if the user inputs something unparsable.

The code thus become:

def problems(operation, symbol, min_op, max_op, sort=False):
    score = 0

    while score < 30:
        a = randint(min_op, max_op)
        b = randint(min_op, max_op)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        try:
            answer = int(raw_input("{} {} {} = ".format(a, symbol, b)))
        except ValueError:
            answer = None

        if answer == result:
            score += 1
            print "Good job. Current score is {}".format(score)
        else:
            print "Oops, the correct answer is {}. Try another one.".format(result)

    print "Good job, {}. You passed this course!".format(name)
    enter_lobby()

Rest of the code

In addition to the speech about using global at the beginning of this post, the call enter_lobby() at the end of the function seems suspicious. What I imagine is that this enter_lobby() function calls one of the various “problems” once, and that's it. Making each problem responsible of returning to it.

This is wrong because it is an implicit recursion. And potentially an infinite one. The lobby calls a problem, which calls a lobby, which calls an other problem, which calls a third lobby… See what I mean? The control flow never return to the first lobby, thus consumming memory to manage the call stack and potentially turning into a RuntimeError: maximum recursion depth exceeded.

This also impairs reusability of your code. Each problem should only be responsible of it's own duty and shouldn't have to know about the rest of the code. If you want to make sure that you stay in the lobby at the end of each problem, you should probably put your enter_lobby code into some kind of infinite loop.

This loop being either within enter_lobby or in the caller body does depend on your intent and the rest of your code, though.

---

¹ Oh, snap!