Skip to main content
Code Review

Return to Answer

added 1242 characters in body
Source Link
llee94
llee94
  • 178
  • 6

Mutually Exclusive Options

Your number of options are fairly small (e.g. 3 options), so this won't be as big of a deal, but in the future you can use elif statements to signify that things are mutually exclusive. It might not seem particularly helpful in this situation, but they can help with readability and in some cases avoid repetition. They're also helpful if there's a chance that the thing you're checking will change value midway through your code (which it shouldn't, but you never know). For example:

# Using elif if status == "1": ... elif status == "2": ... else: ... # If status neither "1" nor "2"

A scenario that elif is helpful might be good if you want to split up a value by score:

score = someNumber letterGrade = "" if score < 60: letterGrade = "F" elif score < 70: # No need to write "score >= 60" because it's implied that it's >= 60 by the "else if" letterGrade = "D" elif score < 80: # No need to write "score >= 70" because it's elif letterGrade = "C" elif score < 90: # No need for "score >= 80" because it's elif letterGrade = "B" else: letterGrade = "A"

General Notes:

General Notes:

Mutually Exclusive Options

Your number of options are fairly small (e.g. 3 options), so this won't be as big of a deal, but in the future you can use elif statements to signify that things are mutually exclusive. It might not seem particularly helpful in this situation, but they can help with readability and in some cases avoid repetition. They're also helpful if there's a chance that the thing you're checking will change value midway through your code (which it shouldn't, but you never know). For example:

# Using elif if status == "1": ... elif status == "2": ... else: ... # If status neither "1" nor "2"

A scenario that elif is helpful might be good if you want to split up a value by score:

score = someNumber letterGrade = "" if score < 60: letterGrade = "F" elif score < 70: # No need to write "score >= 60" because it's implied that it's >= 60 by the "else if" letterGrade = "D" elif score < 80: # No need to write "score >= 70" because it's elif letterGrade = "C" elif score < 90: # No need for "score >= 80" because it's elif letterGrade = "B" else: letterGrade = "A"

General Notes:

Source Link
llee94
llee94
  • 178
  • 6

Adding to what Simon said above,

Unused Loop

No need for the loop in get_bool_input; as it's written, regardless of what the user decides, the function will break out, whether it be by returning a value or with sys.exit().

Unnecessary sys.exits

That said, you have lots of sys.exits--instead of exiting the program altogether, which would force the user to restart everything and retype all the information, you could have a loop like this:

 # Selecting the difficulty level level_of_difficulty = ... while level_of_difficulty not in (1, 2, 3): print("That is not a valid level of difficulty, please try again") level_of_difficulty = ... # try it again ... # Selecting class number class_number = input(...) while class_number not in (1, 2, 3): print("That is not a valid class") class_number = input(...) # try it again

Repetition of Prompting

Echoing what Simon said in the other answer, you'll notice how these are exactly the same snippets of code with the only difference being the values that they can be. You could try writing a function that looks something like this:

def promptSelection(valid_values, selection_type, prompt): result = input(prompt) while result not in valid_values: print("Sorry, that's not a valid {0}".format(selection_type)) result = input(prompt) return result

There is a little bit of repetition because of Python's lack of in-line assignment/return (i.e. You can't while result = input(...):), but it might make it easier.

Concatenation vs. string.format()

The parts where you concatenate a lot of strings you can try this instead for readability. For example:

# Old f.write("\n" + str(name) + " scored " + str(score) + ...) # New f.write("\n{0} scored {1} on difficulty level {2}".format(name, score, level_of_difficulty))

The documentation for this can be found here: https://docs.python.org/release/3.1.3/library/stdtypes.html#str.format

Bug Potential

In the part where you define class_number, it's defined as a string, but you compare to integers in the next line if class_number not in (1, 2, 3)

Single Source of Truth

This one is fairly trivial in this particular situation but for future reference, for the part where you find the random numbers you could do this instead:

# This goes at the top of your code LEVEL_ONE_NUMBER = 10 LEVEL_OTHER_NUMBER = 20 ... # Assigning the numbers for the problem max_range = LEVEL_ONE_NUMBER if level_of_difficulty != 1: max_range = LEVEL_OTHER_NUMBER number_1 = random.randrange(1, max_range) number_2 = random.randrange(1, max_range)

That way you only have to change one value (LEVEL_ONE or LEVEL_OTHER_NUMBER), and you can use them elsewhere in your code should you so desire. This is typically called using single source of truth.

General Notes:

  • Try putting the code that's floating (starting at status = ...) and putting it into a function. Even though this is a fairly small program, it's good to compartmentalize things so if you decide to expand on this to say, be more than just a maths quiz, you don't have to rewrite everything.
  • Comments around parts of your code that serve a specific purpose can help with clarity. For example, you could do something like # Selecting name when you prompt for a name, # Choosing numbers for problem when you assign number_1 and number_2, and other similar situations.