4
\$\begingroup\$

In the process of trying to learn python, and also help my daughter with some elementary math practice over the summer, I'm trying to write a simple math quiz which will reinforce what she learned over the last year. I am using the following, which works okay:

import random
import operator

def quiz():
    print('Welcome! This is a 42 question quiz!\n')
    score = 0
    for i in range(42):
        correct = askQuestion()
        if correct:
            score += 1
            print('Correct!')
            print('Score', score, '\n')
        else:
            print('Incorrect!')
            print('Score', score, '\n')

    print('Your score was {}/42'.format(score))

def askQuestion():
    answer = randomCalc()
    guess = float(input())
    return guess == answer

def randomCalc():
    ops = {'+':operator.add,
            '-':operator.sub}
    num1 = random.randint(0,20)
    num2 = random.randint(0,20)
    op = random.choice(list(ops.keys()))
    answer = ops.get(op)(num1,num2)
    print('What is {} {} {}?'.format(num1, op, num2))
    return answer

quiz()

The problem that I have is that this quiz asks subtraction questions which return negative numbers. As my daughter is in elementary school, I would need the questions to focus on basic subtraction which return non-negative numbers only. I don't need to go that far off the rails yet.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Micro-review: comparing floating-point values with == is risky in any language, due to precision issues. \$\endgroup\$ Jun 22, 2021 at 8:52

2 Answers 2

4
\$\begingroup\$

You made a wise choice wanting to tango with the snake. Just as a mean teacher I will pick apart your code in excruciating detail. I really like your code for a beginner. It is clearly structured, it works and your naming is good. With that being said, you do make some classical beginner mistakes


PEP 8 - Style Guide for Python Code

Just as learning a new language it is important to learn its grammar. Your speech can be technically correct and understandable, but one can tell it is not your mother tongue. The same can be said for your code. It looks like you have experience from a different language, and I would recommend getting familiar with Python's grammar.

PEP 8 recommends the following naming-conventions:

  • CAPITALIZED_WITH_UNDERSCORES for constants
  • UpperCamelCase for class names
  • lowercase_separated_by_underscores for other names

So your functions should not follow cammelcase.


magic numbers

A concept you might find amusing is magic numbers. This is known as a code smell; something which should be avoided when programming. Programming is about making life easier, and to automate the boring stuff. Hardcoding numbers goes against this, and therefore should be avoided. The solution is to extract these numbers into constants following the PEP8 naming-conventions.

QUESTIONS = 42

def quiz():
    print(f'Welcome! This is a {QUESTIONS} question quiz!\n')
    score = 0
    for i in range(QUESTIONS):

f-strings

Did you notice the f in the last code snippet? f-strings is the new way of formating strings in Python, and you should usually use them. For instance

print('What is {} {} {}?'.format(num1, op, num2))

becomes

print(f'What is {num1} {op} {num2}?')

Which is easier to read.


KISS (Keep it simple stupid)

While I have nothing againt using libraries or the operator module, I would suggest sticking to the basics when starting out. This part is heavily opinionated, but I would suggest not using code you do not understand. If you come back to the code half a year later, will you still remember the intricacies of op?

You already know how to use if-else so you could have done

operator_choice = random.randint(0, 1)
if operator_choice == 0:
    answer = num1 - num2
elif operator_choice == 1:
    answer = num1 + num2

handling user input

Never assume that your users input things exactly in the format you want.

def askQuestion():
    answer = randomCalc()
    guess = float(input())
    return guess == answer

Here you need to do some checks to make sure the guess is sanitized. What happens if the user inputs 2*3? or exit? A vague idea is something like

def get_valid_user_input():
    try:
        return float(input())
    except ValueError:
        print('Ooops, you need to input a number!')

if __name__ == "__main__":

Put the parts of your code that are the ones calling for execution behind a if __name__ == "__main__":. This way you can import this python module from other places if you ever want to, and the guard prevents the main code from accidentally running on every import.


negative numbers

I gave you two suggestions to enforce positive numbers:

num1 = random.randint(0,20)
num2 = random.randint(0,20)
num1, num2 = max(num1, num2), min(num1, num2)

or

num1 = random.randint(0,20)
num2 = random.randint(0,num1)

I would recommend trying out these two solutions and try to figure out the difference. As a mathematician there is a difference in the type of numbers you get. Note that the first one also could have been implemented as

num1 = random.randint(0, MAX_NUM1)
num2 = random.randint(0, MAX_NUM2)
if num1 < num2:
    num1, num2 = num2, num1

If you are not that comfortable with min and max.


Round two

I looked over your code again and found a few more things which I found odd. Remember how we talked about grammar at the start? It is somewhat peculiar when you say "this code does X" when it really does "X and Y" or even worse just "Y".

def askQuestion():
    answer = randomCalc()
    guess = float(input())
    return guess == answer

This is not code that's just asking a question. It (1) takes in an answer from randomCalc, (2) takes in a guess from the user, and (3) checks whether the guess from the user matches the answer. You are not really interested in asking a question, that would be more akin to

def askQuestion(question):
    print(question)

What you are interested in is getting feedback from the user. So something akin to get_guess_from_player is probably a better name. Similarly randomCalc is not just an calculator! It also asks the question!

def randomCalc():
    .
    .
    .
    print('What is {} {} {}?'.format(num1, op, num2))

Lastly you are doing something like

for i in range(42):

Which from a "grammar" (Pythonic way) should be

for _ in range(42):

since you never use the _. However, since you never use the for loop, we can instead use something smarter.


suggestions

A clearer structure would be something like this: we define a function that only generates the question. We extract the magic numbers into constants at the start of the code

OPERATORS = ["+", "-"]
MAX_NUMBER = 20
QUESTIONS = 42

def generate_question():
    operator = random.choice(OPERATORS)
    num1 = random.randint(0, MAX_NUMBER)
    num2 = random.randint(0, MAX_NUMBER)
    if num2 > num1:
        num1, num2 = num2, num1
    return f"{num1} {operator} {num2}"

We create a function that only gets a valid guess from the user.

def get_guess():
    try:
        return float(input("> "))
    except ValueError:
        print("Ooops, you need to write in a number!")

Some fancy f-strings and a bit of cleanup the main function looks something like this.

def quiz():
    print(f"Welcome! This is a {QUESTIONS} question quiz!\n")
    score = 0
    for _ in range(QUESTIONS):
        print(question)
        guess = get_guess()
        if guess == eval(question):

Be wary; dragons lie ahead

A trap many new programmers falls into is premature optimization. However, I can not avoid noticing that the logic in the code could be improved. If you are asking multiple questions; why not generate all the numbers and operators at the start? It would lead to a cleaner and faster code. Notice how our main loop now reads

for question in generated_questions():

Much more Pythonic! I also included Toby's suggestion even if it is 100% overkill when doing basic arithmetic.

import numpy as np

QUESTIONS = 42
BAN_NEGATIVE_NUMBERS = True
OPERATORS = ["-", "+"]
MAX_NUMBER, DECIMALS = 20, 2
MAX_ERROR = 10 ** (1 - DECIMALS)

def generate_questions():
    operators = np.random.choice(a=OPERATORS, size=(QUESTIONS, 1))
    numbers = np.random.choice(a=MAX_NUMBER, size=(QUESTIONS, 2))
    for operator, (num1, num2) in zip(operators, numbers):
        if num1 < num2 and operator == "-" and BAN_NEGATIVE_NUMBERS:
            num1, num2 = num2, num1
        yield f"{num1} {operator[0]} {num2}"

def get_guess():
    try:
        return float(input("> "))
    except ValueError:
        print("Ooops, you need to write in a number!")
        return float("Inf")

def quiz(score=0):
    print(f"Welcome! This is a {QUESTIONS} question quiz!\n")
    for question in generate_questions():
        print(question)
        guessed_right = abs(get_guess() - eval(question)) <= MAX_ERROR
        score += 1 if guessed_right else 0
        print("Correct!" if guessed_right else "Incorrect", "\nScore", score, "\n")
    print(f"Your score was {score}/{QUESTIONS} = {round(100*score/QUESTIONS, 2)}")

if __name__ == "__main__":
    quiz()
\$\endgroup\$
7
  • \$\begingroup\$ Overall agree, but what you're calling grammar is not grammar - it's style. Grammar and syntax are loosely interchangeable, and refer to what will or will not parse. Style does not affect whether code will parse, and is more about convention and legibility. \$\endgroup\$
    – Reinderien
    Jun 22, 2021 at 17:16
  • \$\begingroup\$ What I meant is that grammar is to a language what style is to a programming language. As you said it compiles (it is understandable), but breaks convention. Sorry if that was confusing, English is far from my first language. \$\endgroup\$ Jun 22, 2021 at 17:23
  • \$\begingroup\$ @N3buchadnezzar and Toby Speight - I appreciate your candor and honest assessment of the code posted here, as well as the corrections you have recommended. I have seen other newbies post questions and the outcome for those folks was unfortunate to say the least. I am reviewing this now and will look to implement and continue testing and if other questions arise, I will be back. Thanks for your assistance! \$\endgroup\$
    – Shane
    Jun 22, 2021 at 18:04
  • 1
    \$\begingroup\$ @Shane I recommend you trying it out! Try to define for instance num1, num2 = 1, 2 and then print them print(num1, num2). After these two lines write num1, num2 = num2, num1 and then print. What happens? \$\endgroup\$ Jun 23, 2021 at 17:29
  • 1
    \$\begingroup\$ Teach them to fish, don't give them the fish. 100% \$\endgroup\$
    – Shane
    Jun 23, 2021 at 17:42
1
\$\begingroup\$

What a beautiful exercise - both for you and your child. In brief, and significantly overlapping with the feedback from @N3buchadnezzar:

  • Don't repeat the constant 42; centralize it
  • Move 20 to a constant
  • lower_snake_case for all of your symbols
  • Factor out common statements to a single place, such as your score print
  • f-strings are more convenient than format calls or multi-argument print calls
  • Centralize your question-related I/O into one method (ask_question), and your computation into another (random_calc)
  • Use PEP484 type hints
  • float is not appropriate here; use int instead
  • Include a main guard
  • You're not actually using OPS for its dictionary features, so just make it a tuple of tuples
  • It's a little mean to mark a question incorrect without showing the actual answer. Consider including that.

Suggested:

import random
import operator
from typing import Tuple

OPS = (
    ('+', operator.add),
    ('-', operator.sub),
)
MAX_TERM = 20


def quiz(n: int) -> None:
    print(f'Welcome! This is a {n} question quiz!\n')
    score = 0

    for _ in range(n):
        score += ask_question()
        print('Score', score, '\n')

    print(f'Your score is {score}/{n}')


def ask_question() -> int:
    num1, op, num2, answer = random_calc()
    guess = int(input(
        f'What is {num1} {op} {num2}? '
    ))

    if guess == answer:
        print('Correct!')
        return 1

    print(f'Incorrect; the answer is {answer}.')
    return 0


def random_calc() -> Tuple[
    int,  # First operand
    str,  # Operator
    int,  # Second operand
    int,  # Answer
]:
    num1 = random.randint(0, MAX_TERM)
    num2 = random.randint(0, MAX_TERM)
    op_name, op = random.choice(OPS)
    answer = op(num1, num2)
    return num1, op_name, num2, answer


if __name__ == '__main__':
    quiz(n=5)

Output:

Welcome! This is a 5 question quiz!

What is 5 - 6? -1
Correct!
Score 1 

What is 8 + 2? 10
Correct!
Score 2 

What is 20 - 6? 14
Correct!
Score 3 

What is 4 + 14? 18
Correct!
Score 4 

What is 16 - 4? 500
Incorrect; the answer is 12.
Score 4 

Your score is 4/5
\$\endgroup\$
4
  • \$\begingroup\$ I agree on most of this, but it might be useful to keep OPS as a dict to guarantee the operators have unique names \$\endgroup\$
    – Sara J
    Jun 22, 2021 at 17:56
  • 1
    \$\begingroup\$ @SaraJ I would agree if the ops are user-defined, but they are not - they're programmer-defined, and so given the simplicity of the data and the low risk I don't see the need. \$\endgroup\$
    – Reinderien
    Jun 22, 2021 at 18:03
  • 1
    \$\begingroup\$ @SaraJ Add to that that you'd not want to use a normal dict, but instead some implementation of a frozendict. Here with tuples you get immutability for free. \$\endgroup\$
    – Reinderien
    Jun 22, 2021 at 18:04
  • 1
    \$\begingroup\$ True, you do get immutability, but as mentioned you do give up uniqueness, and I don't see much reason to pick one over the other here (both are good of course). But yeah, for simple use of simple data it doesn't matter a whole lot either way \$\endgroup\$
    – Sara J
    Jun 22, 2021 at 18:14

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.