Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 speed up.

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.

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.

Added more information about why using a global `i` is bad.
Source Link
Peilonrayz
  • 44.4k
  • 7
  • 80
  • 157

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 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.

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.

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:

for i in range(10):
 test(i)

This is as tracking one letter variable names is hard, and annoying. Even more so if you are using an idiom unidiomatically.

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.

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.

Source Link
Peilonrayz
  • 44.4k
  • 7
  • 80
  • 157

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:

for i in range(10):
 test(i)

This is as tracking one letter variable names is hard, and annoying. Even more so if you are using an idiom unidiomatically.

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))

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.

lang-py

AltStyle によって変換されたページ (->オリジナル) /