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?

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.