3
\$\begingroup\$

I am doing a controlled assessment and one of the tasks is that we have to sort the saved pupils scores (numerically), and their names (alphabetically) in a .txt file. Any other improvements to the code would also be greatly appreciated.

import random
classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber == "1":
        print()
        classcheck = True
    elif classnumber == "2":
        print()
        classcheck = True
    elif classnumber == "3":
        print()
        classcheck = True
    else:
        print("that is not a valid class number")


student1name = input("please enter your name")
student1score = 0



for i in range(10):
    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)


    operators = ['+','-','*']
    op_number  = random.randint(0,2)
    op_sym = operators[op_number]
    op = op_number

    if op == 0:
        ans = question1no1 + question1no2
    elif op == 1:
        ans = question1no1 - question1no2
    elif op == 2:
        ans = question1no1 * question1no2




    print(str(question1no1), str(op_sym) , str(question1no2))

    question = "what is "+str(question1no1) + str(op_sym) +                                             str(question1no2)+"?"

    student1answer1 = input(question)

    print (student1answer1)

    if str(student1answer1) == str(ans):
        print("congrats you got the answer right")
        student1score = student1score + 1

    else:
        print("sorry you got the answer wrong")

print("your score was " + str(student1score) + " out of 10")

if classnumber == "1":
    class1score = open("class1score.txt", "a")
    class1score.write("\n" + student1name + (" ") + str(student1score))
    class1score.close()


if classnumber == "2":
    class2score = open("class2score.txt", "a")
    class2score.write("\n" + student1name + (" ") + str(student1score))
    class2score.close()



if classnumber == "3":

    class3score = open("class3score.txt", "a")
    class3score.write("\n" + student1name + (" ") + str(student1score))
    class3score.close()
\$\endgroup\$

migrated from stackoverflow.com Jan 19 '16 at 14:40

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ sorry about the indentation i wasn't sure about how to make a code block the actual code has proper indentation. \$\endgroup\$ – Greg Knight Jan 19 '16 at 13:08
  • \$\begingroup\$ Has the sorting anything to do with the code above? Do you want to write the files already sorted or sort afterwards (and potentially keep the first order)? \$\endgroup\$ – Hubert Grzeskowiak Jan 19 '16 at 13:13
  • 2
    \$\begingroup\$ To indent the code block in SE you select the code block, and then press ctrl + k. Alternately in an editor of your choice prepend each line with four spaces. \$\endgroup\$ – Peilonrayz Jan 19 '16 at 16:49
  • \$\begingroup\$ @GregKnight: the 'insert into file sorted' part is only relevant for the end of the code. Maybe make a new post for it: "given a file, how do I add a line and keep sort order?". The rest of the code has almost nothing to do with your question. \$\endgroup\$ – Sjoerd Job Postmus Jan 21 '16 at 7:57
3
\$\begingroup\$

My first suggestion would be to use some extra functions. However, let's look at it in several parts.

First, the getting of the class number. You have

classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber == "1":
        print()
        classcheck = True
    elif classnumber == "2":
        print()
        classcheck = True
    elif classnumber == "3":
        print()
        classcheck = True
    else:
        print("that is not a valid class number")

The if statements are quite the same. By using set-membership, we can reduce the duplicate code.

classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber in {"1", "2", "3"}:
        classcheck = True
    else:
        print("that is not a valid class number")

Furthermore, writing while foo == False: is not really idiomatic Python. Better would be while foo:. But, in this case you're trying to emulate a do {...} while (...) loop. I'd suggest writing it as follows:

while True:
    classnumber = input("what class are you in")
    if classnumber in {"1", "2", "3"}:
        break
    print("that is not a valid class number")

The asking of the name is quite obvious, let's move to the part where the questions get asked:

for i in range(10):

The variable i does not actually get used. It's a minor nitpick, but convention has it that you should write

for _ in range(10):

instead.

    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)

What is question1no1 referring to? This will also be executed for question 2 to 10. Maybe operand_left and operand_right would be better names? Leaving them be for now, but it's something you can ponder about.

Next, the creation of the 'puzzle'/question.

    operators = ['+','-','*']
    op_number  = random.randint(0,2)
    op_sym = operators[op_number]
    op = op_number

    if op == 0:
        ans = question1no1 + question1no2
    elif op == 1:
        ans = question1no1 - question1no2
    elif op == 2:
        ans = question1no1 * question1no2

There are a few lines between the definition of the operators, and the calculation of the desired result. First suggestion: use random.choice(operators) instead of random.randint(0, 2)

    operators = ['+','-','*']
    op_sym = random.choice(operators)

    if op_sym == '+':
        ans = question1no1 + question1no2
    elif op_sym == '-':
        ans = question1no1 - question1no2
    elif op_sym == '*':
        ans = question1no1 * question1no2

Already a lot clearer, no? Still, I've typed the + sign 3 times here. By using the operator module, I could write:

import operator
...
...

    operators = [
        ('+', operator.add),
        ('-', operator.sub),
        ('*', operator.mul),
    ]
    op_sym, op_func = random.choice(operators)
    ans = op_func(question1no1, question1no2)

Adding a new operator would be a simple method of adding another line in the list above.

I'll assume the explicit print statements are a bit of debugging work, and ignore those. Ideally you'd remove them.

Look at how you write the question.

    question = "what is "+str(question1no1) + str(op_sym) + str(question1no2)+"?"

There are so many things going on, it is a bit worry-some. Also, you might want to add a space around the operator, causing the need for yet some more work. By using string formatting (https://docs.python.org/3.5/library/stdtypes.html#str.format), you can make it a bit simpler:

    question = "what is {} {} {}?".format(question1no1, op_sym, question1no2)

Now, we get to student1answer1. Why not just answer? (Or given_answer, and rename ans to expected_answer).

As for checking the results:

    if str(student1answer1) == str(ans):
        print("congrats you got the answer right")
        student1score = student1score + 1

    else:
        print("sorry you got the answer wrong")

You can remove the empty line between the if and the else block. But more importantly, instead of

        student1score = student1score + 1

you can write

        student1score += 1

with the same effect.

At the end, we also see duplicated code regarding to the class numbers:

if classnumber == "1":
    class1score = open("class1score.txt", "a")
    class1score.write("\n" + student1name + (" ") + str(student1score))
    class1score.close()


if classnumber == "2":
    class2score = open("class2score.txt", "a")
    class2score.write("\n" + student1name + (" ") + str(student1score))
    class2score.close()



if classnumber == "3":

    class3score = open("class3score.txt", "a")
    class3score.write("\n" + student1name + (" ") + str(student1score))
    class3score.close()

It should be quite obvious that the only difference in these statements is the filename. "Easy" fix:

if classnumber == "1":
    filename = "class1score.txt"
if classnumber == "2":
    filename = "class2score.txt"
if classnumber == "3":
    filename = "class3score.txt"

class_score = open(filename, "a")
class_score.write("\n" + student1name + " " + str(student1score))
class_score.close()

But the if is now still very suspicious. Let's replace those 3 if-statements with 1 statement:

filename = "class" + classnumber + "score.txt"

or

filename = "class{}score.txt".format(classnumber)

(my preference is the second).

Finally: please use capitalization when writing strings for the end-user. So, instead of "what class are you in", write "What class are you in? ". (The final space is to make the question look even better when entering the data, but that's merely convention.)

After making the changes, I ended up with

import operator
import random


while True:
    classnumber = input("What class are you in? ")
    if classnumber in {"1", "2", "3"}:
        break
    print("That is not a valid class number.")


student1name = input("Please enter your name: ")
student1score = 0



for _ in range(10):
    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)

    operators = [
        ('+', operator.add),
        ('-', operator.sub),
        ('*', operator.mul),
    ]
    op_sym, op_func = random.choice(operators)
    ans = op_func(question1no1, question1no2)

    question = "What is {} {} {}? ".format(question1no1, op_sym, question1no2)

    student1answer1 = input(question)

    if str(student1answer1) == str(ans):
        print("Congrats, you got the answer right!")
        student1score += 1
    else:
        print("Sorry, you got the answer wrong.")

print("Your score was " + str(student1score) + " out of 10.")

filename = "class{}score.txt".format(classnumber)
class_score = open(filename, "a")
class_score.write("\n" + student1name + " " + str(student1score))
class_score.close()

Also, I'd like to change the last write to have the "\n" at the end instead of at the beginning, as it's convention to end a line with a "\n" instead of starting one with it. But that's up to you, as it would change the semantics of the script.

\$\endgroup\$
  • \$\begingroup\$ Good review! Out of curiosity, why use a set for in {'1', '2', '3'}? I usually use a tuple but mostly as convention. \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 10:00
  • \$\begingroup\$ @SuperBiasedMan: merely a preference. Also, I remember doing some benchmarking, and thought sets were faster. Tuples provide fast iteration, but sets faster membership tests. (If I remember correctly). \$\endgroup\$ – Sjoerd Job Postmus Jan 21 '16 at 11:02
  • \$\begingroup\$ Sets are faster for the test, but slower to create since they perform the hashing. But the performance is always going to be context dependent and in performance sensitive cases creating the set as a constant would probably be the quickest. \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 11:06
  • 1
    \$\begingroup\$ @SuperBiasedMan & SjoerdJobPostmus Sets are generally faster, in fact, it seems raw strings are generally faster than tuples... my times. \$\endgroup\$ – Peilonrayz Jan 21 '16 at 11:28
  • \$\begingroup\$ Ah, I get different times on my IDLE Python 2.7 (where tuples beat sets every time without setup), so it's obviously environment dependent too. That said, strings are clear winner for me too. \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 11:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.