Random maths quiz with sorting

Question

import random
import operator
import pickle
classname = ""
while True:
 classpick = input("Enter class name (\"A,B,C\") ").upper()
 try:
 if classpick in ["A","B","C"]:
 classname = "class"+classpick+".txt"
 break
 else:
 raise ValueError
 except ValueError:
 print("Invalid value. Please enter a letter A,B or C.")
 break
with open(classname + ".txt", "rb") as handle:
 data = pickle.load(handle)
def test():
 num1 = random.randint(1, 10)
 num2 = random.randint(1, num1)
 ops = {
 '+': operator.add,
 '-': operator.sub,
 '*': operator.mul,
 }
 ops_keys = list(ops.keys()) ##=> ['+', '*', '-']
 ops_key_choice = random.choice(ops_keys) #e.g. '+' 
 operation = ops[ops_key_choice] #e.g. operator.add
 correct_result = operation(num1, num2)
 question = ("Q{}. What is {} {} {}?".format(i+1,num1, ops_key_choice, num2))
 print(question)
 while True:
 try:
 user_answer= int(input("Answer: "))
 except ValueError:
 print ("That is not a valid answer")
 print (question)
 else:
 break
 if user_answer != correct_result:
 print ("Incorrect. The right answer is {}".format(correct_result))
 return False
 else:
 print("Correct!")
 return True
###
def sort_dict(data):
 while True:
 sortpick = str(input('''Enter a number to sort by:
 \n1. Alphabetically
 \n2. Highest Score
 \n3. Highest Average Score'''))
 try:
 if sortpick in ["1","2","3"]:
 break
 else:
 raise ValueError
 except ValueError:
 print("Invalid value. Please enter a number in the range of 1-3.")
 break
 if sortpick == "1": #Alphabetical
 for x, y in sorted(data.items()):
 print(x,y)
 elif sortpick == "2": #Highest Score
 for k,v in sorted(((max(data[k]), k) for k in data), reverse=True): 
 print('{} : {}'.format(v,k))
 else: #Highest Average Score
 sort = sorted(data, key=lambda k: sum(data[k]) / len(data[k]), reverse=True)
 for i in sort:
 print("{} : {}".format(i,
 round(sum(data[i])/len(data[i]),1 ) ))
username = input("What is your name? ").upper()
print("Hi {}! Welcome to the Primary School Arithmetic quiz...".format(username))
correct_count = 0
question_count = 10
for i in range(question_count):
 if test():
 correct_count +=1
if username in data:
 if len(data[username]) == 3:
 del(data[username][0])
 data[username].append(correct_count)
 else:
 data[username].append(correct_count)
else:
 data[username] = [correct_count]
print("{}: You got {}/{} questions correct. {}".format(
 username, 
 correct_count, 
 question_count,
 'Great job!' if (correct_count>7) else 'Better luck next time...'
))
with open(classname+".txt", "wb") as handle:
 pickle.dump(data, handle)
print("{}: {}".format(k, v) for k, v in data.items()) #prints whole dict
for k, v in data.items():
 print("{}: {}".format(k, v))
while True:
 try:
 sort_or_not = input("Would you like to sort the class results?? Y/N").upper()
 if sort_or_not.isalpha() == True:
 break
 else:
 raise ValueError
 except ValueError:
 print ("That is not a valid input")
 break
if sort_or_not == "Y":
 print("")
else:
 quit()
sort_dict(data)

The code above is designed to ask users a set of 10 randomized maths questions then save the score to a file and sort the scores.

How can I make this code more efficient?

• Vogel612: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

You should really use more functions, for a little more to type def fn_name and fn_name() you can get way more readability. You can also get quite a significant speed up.

For example the code to pick a class name can be wrapped in one.

def pick_class_name():
 classname = ""
 while True:
 classpick = input("Enter class name (\"A,B,C\") ").upper()
 try:
 if classpick in ["A","B","C"]:
 classname = "class"+classpick+".txt"
 break
 else:
 raise ValueError
 except ValueError:
 print("Invalid value. Please enter a letter A,B or C.")
 break
 return classname

This can be improved quite significantly. The only time that the try will raise a ValueError is when you explicitly raise ValueError. This basically means that your except should be your else.

def pick_class_name():
 classname = ""
 while True:
 classpick = input("Enter class name (\"A,B,C\") ").upper()
 if classpick in ["A","B","C"]:
 classname = "class"+classpick+".txt"
 break
 else:
 print("Invalid value. Please enter a letter A,B or C.")
 break
 return classname

The above clearly shows the while True is pointless, along with all the breaks. Instead you could remove the break on the else and change the class_name = into a return.

def pick_class_name():
 while True:
 classpick = input("Enter class name (\"A,B,C\") ").upper()
 if classpick in ["A","B","C"]:
 return "class" + classpick + ".txt"
 else:
 print("Invalid value. Please enter a letter A,B or C.")

You do these try: if: ... else: raise: except: blocks rather than just a simple if ... else, in a few places, mostly in while loops, that can probably be changed like the above.

---

You use test in a for loop but rely on the global i to output the correct question number. This is just bad, first for i in ... is only good if you use that i in a small loop like:

def test(question_number):
 #...
 # Use `question_number` rather than `i`
for i in range(10):
 test(i)

This is as tracking global and local one letter variable names is hard and annoying. Even more so if you are using an idiom unidiomatically. So you would want to use question_number rather than i in this case. You however don't want to change i to question_number in the loop, as you'll still have to remember global variable.

This would also allow you to change that block of code into a one-liner:

correct_count = sum(map(test, range(question_count)))
correct_count = sum(test(i) for i in range(question_count))

Where if you used the first one in your current code you will get an error or the question number won't change. And so it's prone to breaking your function depending on how it's used.

---

Your code is quite hard to read, as you rely on global variables in your functions, and you don't have many functions. To both make it more efficient and easier to read you should add more functions.

• user94917: Thanks for the advice on the functions, I've implemented that now. Isn't a try and catch loop better for finding errors? And I didn't quite understand the code for your last point on the for loop, do you mind explaining?
• Peilonrayz: @D3107 Try catch is good, but not when you could do a if else instead. I.e. try: user_answer= int(input("Answer: ")) except ValueError: is fine. I'll re-word the for loop bit. (Not enough characters here)