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:
- The operation to perform (both taking two arguments);
- 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!
while
loop calls other smaller functions that return the correct answer and the message for each math operation) but not so much by switching to using OOP. \$\endgroup\$ – jcfollower Dec 3 '15 at 21:46