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.
2 Answers 2
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 constantsUpperCamelCase
for class nameslowercase_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()
-
\$\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– Reinderien2021年06月22日 17:16:32 +00:00Commented 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\$N3buchadnezzar– N3buchadnezzar2021年06月22日 17:23:33 +00:00Commented 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– Shane2021年06月22日 18:04:39 +00:00Commented 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 themprint(num1, num2)
. After these two lines writenum1, num2 = num2, num1
and then print. What happens? \$\endgroup\$N3buchadnezzar– N3buchadnezzar2021年06月23日 17:29:51 +00:00Commented Jun 23, 2021 at 17:29 -
1\$\begingroup\$ Teach them to fish, don't give them the fish. 100% \$\endgroup\$Shane– Shane2021年06月23日 17:42:26 +00:00Commented Jun 23, 2021 at 17:42
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; useint
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
-
\$\begingroup\$ I agree on most of this, but it might be useful to keep
OPS
as adict
to guarantee the operators have unique names \$\endgroup\$Sara J– Sara J2021年06月22日 17:56:34 +00:00Commented 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– Reinderien2021年06月22日 18:03:14 +00:00Commented 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– Reinderien2021年06月22日 18:04:15 +00:00Commented 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– Sara J2021年06月22日 18:14:14 +00:00Commented Jun 22, 2021 at 18:14
==
is risky in any language, due to precision issues. \$\endgroup\$