Expired---------------
$begingroup$
This post has been deleted #001010101010100200
print("This code is expired")
print("See receipt #001010101010100200
print("Completed")
python-3.x
$endgroup$
|
show 1 more comment
$begingroup$
This post has been deleted #001010101010100200
print("This code is expired")
print("See receipt #001010101010100200
print("Completed")
python-3.x
$endgroup$
1
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)I'm experiencing some issues
please describe these, and why you have trouble resolving them.
$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36
|
show 1 more comment
$begingroup$
This post has been deleted #001010101010100200
print("This code is expired")
print("See receipt #001010101010100200
print("Completed")
python-3.x
$endgroup$
This post has been deleted #001010101010100200
print("This code is expired")
print("See receipt #001010101010100200
print("Completed")
python-3.x
python-3.x
edited 20 mins ago
ThatNewbieCoder101
asked Feb 4 at 23:23
ThatNewbieCoder101ThatNewbieCoder101
12
12
1
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)I'm experiencing some issues
please describe these, and why you have trouble resolving them.
$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36
|
show 1 more comment
1
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)I'm experiencing some issues
please describe these, and why you have trouble resolving them.
$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36
1
1
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)
I'm experiencing some issues
please describe these, and why you have trouble resolving them.$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)
I'm experiencing some issues
please describe these, and why you have trouble resolving them.$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36
|
show 1 more comment
1 Answer
1
active
oldest
votes
$begingroup$
Code organisation
Having many functions defined in the middle of the core loop make things very hard to understand. It would be much easier and much more conventional to define all functions before the loop. One of the issues you might encounter is that functions will rely on variables they know nothing about such as players
. A simple solution could be to add these as parameters.
Also, main
is a very confusing name for a function which is not the first thing launched. I guess play_turn
would be a better name.
At this stage, we'd have something like:
Disclaimer: I've changed various details (removed fstrings because I don't have Python 3.6 available, etc)
import random, time
SIMU = True # hack for testing purposes
def display_intro():
title = "** A Simple Math Quiz **"
print("*" * len(title))
print(title)
print("*" * len(title))
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
print(menu_list[0])
print(menu_list[1])
print(menu_list[2])
print(menu_list[3])
print(menu_list[4])
def display_separator():
print("-" * 24)
def get_user_input():
user_input = int(input("Enter your choice: "))
while user_input > 5 or user_input <= 0:
print("Invalid menu option.")
user_input = int(input("Please try again: "))
else:
return user_input
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
return count
else:
print("Incorrect.")
return count
def get_user_solution(problem, player):
if not SIMU:
time.sleep(random.randint(1,8))
then = time.time()
print(problem, end="")
result = int(input(" = "))
reaction_time = time.time()-then
print(" ms")
player["points"] += reaction_time
return result
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
problem = str(number_one) + " + " + str(number_two)
solution = number_one + number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 2:
problem = str(number_one) + " - " + str(number_two)
solution = number_one - number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 3:
problem = str(number_one) + " * " + str(number_two)
solution = number_one * number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
else:
problem = str(number_one) + " // " + str(number_two)
solution = number_one // number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
def display_result(total, correct):
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
if total == 0:
percentage = 0
print("You answered", total, "questions with", correct, "correct.")
print("Your score is ", percentage, "%. Thank you.", sep = "")
winner = min(players,key=lambda a: a['points'])['name']
print("The winner is...")
time.sleep(0.5)
print("{winner}!")
def play_turn(player):
if SIMU:
print("It is now 's turn.nBe ready.")
else:
input("It is now 's turn.nPress enter when ready.")
display_intro()
display_menu()
display_separator()
option = get_user_input()
total = 0
correct = 0
while option != 5:
total = total + 1
correct = menu_option(option, correct, player)
option = get_user_input()
print("Exit the quiz.")
display_separator()
display_result(total, correct)
def play_round(players):
print("-"*20 + "nIt is now round !n" + "-"*20)
for player in players:
play_turn(player)
def play_game():
if SIMU:
players = [{'name': name, 'points': 0} for name in ('foo', 'bar', 'foobar')]
nb_rounds = 1
else:
players = [{
'name':input("Player: "),
'points':0
} for i in range(int(input("How many players? ")))]
nb_rounds = int(input("How many rounds? "))
for i in range(0, nb_rounds):
play_round(players)
play_game()
Now, it it easier to improve these functions.
Improve display_intro
We could compute the length and the border string in a single place:
def display_intro():
title = "** A Simple Math Quiz **"
border = "*" * len(title)
print(border)
print(title)
print(border)
Improve display_menu
It is a good idea to have the menu in a proper data structure. It would be even better too iterate over it instead of using indices: it would be more concise, more efficient and easier to maintain (you do not have to remember to add/remove lines when you add/remove elements from the menu).
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
for menu_item in menu_list:
print(menu_item)
Also, we could have the numbers generated automatically but we'll come back to this.
Improve check_solution
You have the same thing returned in both branches. You could have it in a single place:
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
else:
print("Incorrect.")
return count
Improving display_result
You could use elif
at the beginning because the 2 cases are mutually exclusive:
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
elif total == 0:
percentage = 0
However, I'd rather have this written:
if total == 0:
percentage = 0
else:
result = correct / total
percentage = round((result * 100), 2)
Improve menu_option
The main thing one can see is that it contains highly duplicated code.
Let's extract out the common part to make things clearer:
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
op_str = " + "
res = number_one + number_two
elif index is 2:
op_str = " - "
res = number_one - number_two
elif index is 3:
op_str = " * "
res = number_one * number_two
else:
op_str = " // "
res = number_one // number_two
problem = str(number_one) + op_str + str(number_two)
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, res, count)
return count
You use is
to compare values. You should actually use ==
at that stage. You'll find more online about the is
operator.
Also, instead of writing that code with multiple conditions, we could try to fit the relevant data in a proper data structure, for instance a dictionnary mapping indices to information about the operation to be performed. The operator
module will be helpful here (you could also use lambda functions).
You'd have something like:
import operator
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
operations = {
1: (" + ", operator.add),
2: (" - ", operator.sub),
3: (" * ", operator.mul),
4: (" // ", operator.floordiv),
}
op_str, op_func = operations[index]
res = op_func(number_one, number_two)
problem = str(number_one) + op_str + str(number_two)
count = check_solution(get_user_solution(problem, player), res, count)
return count
Now, we can think about the real meaning of the keys of that dict: they correspond to the user inputs we display in display_menu
.
If we want to add or remove a supported mathematical operation, it is very hard to get it right. We should try to use a Single Source of Truth.
Then we also realise that the same information is also used in get_user_input
along with the input to exit. It would make sense to get rid of that magic number which appears also in play_turn
.
We could write something like:
OPERATIONS = {
1: (" + ", "Addition", operator.add),
2: (" - ", "Substraction", operator.sub),
3: (" * ", "Multiplication", operator.mul),
4: (" // ", "Integer Division", operator.floordiv),
}
EXIT_KEY = 5
def display_menu():
for k, v in OPERATIONS.items():
op_str, op_name, op_func = v
print(str(k) + ". " + op_name)
print(str(EXIT_KEY) + ". Exit")
def get_user_input():
user_input = int(input("Enter your choice: "))
while True:
if user_input in OPERATIONS or user_input == EXIT_KEY:
return user_input
print("Invalid menu option.")
user_input = int(input("Please try again: "))
Now, adding or removing an operation is much more trivial. Also, if we want to start to use letters instead of numbers for the menu options, it is quite easy to do as well.
Details
total = total + 1
can be written total += 1
.
Small functions and responsabilities
Instead of having check_solution
called with a count
value and returning the value incremented or not, we could ensure the value just returns a boolean and the calling function is reponsible for using it.
def menu_option(index, count, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
if check_solution(get_user_solution(problem, player), res):
count += 1
return count
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
Now, the same logic actually applies to menu_option
:
def menu_option(index, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
return check_solution(get_user_solution(problem, player), res)
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
...
while option != EXIT_KEY:
total += 1
if menu_option(option, player):
correct += 1
option = get_user_input()
At that stage, I start to feel that the function names could be improved.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212884%2fexpired%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
Code organisation
Having many functions defined in the middle of the core loop make things very hard to understand. It would be much easier and much more conventional to define all functions before the loop. One of the issues you might encounter is that functions will rely on variables they know nothing about such as players
. A simple solution could be to add these as parameters.
Also, main
is a very confusing name for a function which is not the first thing launched. I guess play_turn
would be a better name.
At this stage, we'd have something like:
Disclaimer: I've changed various details (removed fstrings because I don't have Python 3.6 available, etc)
import random, time
SIMU = True # hack for testing purposes
def display_intro():
title = "** A Simple Math Quiz **"
print("*" * len(title))
print(title)
print("*" * len(title))
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
print(menu_list[0])
print(menu_list[1])
print(menu_list[2])
print(menu_list[3])
print(menu_list[4])
def display_separator():
print("-" * 24)
def get_user_input():
user_input = int(input("Enter your choice: "))
while user_input > 5 or user_input <= 0:
print("Invalid menu option.")
user_input = int(input("Please try again: "))
else:
return user_input
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
return count
else:
print("Incorrect.")
return count
def get_user_solution(problem, player):
if not SIMU:
time.sleep(random.randint(1,8))
then = time.time()
print(problem, end="")
result = int(input(" = "))
reaction_time = time.time()-then
print(" ms")
player["points"] += reaction_time
return result
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
problem = str(number_one) + " + " + str(number_two)
solution = number_one + number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 2:
problem = str(number_one) + " - " + str(number_two)
solution = number_one - number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 3:
problem = str(number_one) + " * " + str(number_two)
solution = number_one * number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
else:
problem = str(number_one) + " // " + str(number_two)
solution = number_one // number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
def display_result(total, correct):
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
if total == 0:
percentage = 0
print("You answered", total, "questions with", correct, "correct.")
print("Your score is ", percentage, "%. Thank you.", sep = "")
winner = min(players,key=lambda a: a['points'])['name']
print("The winner is...")
time.sleep(0.5)
print("{winner}!")
def play_turn(player):
if SIMU:
print("It is now 's turn.nBe ready.")
else:
input("It is now 's turn.nPress enter when ready.")
display_intro()
display_menu()
display_separator()
option = get_user_input()
total = 0
correct = 0
while option != 5:
total = total + 1
correct = menu_option(option, correct, player)
option = get_user_input()
print("Exit the quiz.")
display_separator()
display_result(total, correct)
def play_round(players):
print("-"*20 + "nIt is now round !n" + "-"*20)
for player in players:
play_turn(player)
def play_game():
if SIMU:
players = [{'name': name, 'points': 0} for name in ('foo', 'bar', 'foobar')]
nb_rounds = 1
else:
players = [{
'name':input("Player: "),
'points':0
} for i in range(int(input("How many players? ")))]
nb_rounds = int(input("How many rounds? "))
for i in range(0, nb_rounds):
play_round(players)
play_game()
Now, it it easier to improve these functions.
Improve display_intro
We could compute the length and the border string in a single place:
def display_intro():
title = "** A Simple Math Quiz **"
border = "*" * len(title)
print(border)
print(title)
print(border)
Improve display_menu
It is a good idea to have the menu in a proper data structure. It would be even better too iterate over it instead of using indices: it would be more concise, more efficient and easier to maintain (you do not have to remember to add/remove lines when you add/remove elements from the menu).
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
for menu_item in menu_list:
print(menu_item)
Also, we could have the numbers generated automatically but we'll come back to this.
Improve check_solution
You have the same thing returned in both branches. You could have it in a single place:
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
else:
print("Incorrect.")
return count
Improving display_result
You could use elif
at the beginning because the 2 cases are mutually exclusive:
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
elif total == 0:
percentage = 0
However, I'd rather have this written:
if total == 0:
percentage = 0
else:
result = correct / total
percentage = round((result * 100), 2)
Improve menu_option
The main thing one can see is that it contains highly duplicated code.
Let's extract out the common part to make things clearer:
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
op_str = " + "
res = number_one + number_two
elif index is 2:
op_str = " - "
res = number_one - number_two
elif index is 3:
op_str = " * "
res = number_one * number_two
else:
op_str = " // "
res = number_one // number_two
problem = str(number_one) + op_str + str(number_two)
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, res, count)
return count
You use is
to compare values. You should actually use ==
at that stage. You'll find more online about the is
operator.
Also, instead of writing that code with multiple conditions, we could try to fit the relevant data in a proper data structure, for instance a dictionnary mapping indices to information about the operation to be performed. The operator
module will be helpful here (you could also use lambda functions).
You'd have something like:
import operator
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
operations = {
1: (" + ", operator.add),
2: (" - ", operator.sub),
3: (" * ", operator.mul),
4: (" // ", operator.floordiv),
}
op_str, op_func = operations[index]
res = op_func(number_one, number_two)
problem = str(number_one) + op_str + str(number_two)
count = check_solution(get_user_solution(problem, player), res, count)
return count
Now, we can think about the real meaning of the keys of that dict: they correspond to the user inputs we display in display_menu
.
If we want to add or remove a supported mathematical operation, it is very hard to get it right. We should try to use a Single Source of Truth.
Then we also realise that the same information is also used in get_user_input
along with the input to exit. It would make sense to get rid of that magic number which appears also in play_turn
.
We could write something like:
OPERATIONS = {
1: (" + ", "Addition", operator.add),
2: (" - ", "Substraction", operator.sub),
3: (" * ", "Multiplication", operator.mul),
4: (" // ", "Integer Division", operator.floordiv),
}
EXIT_KEY = 5
def display_menu():
for k, v in OPERATIONS.items():
op_str, op_name, op_func = v
print(str(k) + ". " + op_name)
print(str(EXIT_KEY) + ". Exit")
def get_user_input():
user_input = int(input("Enter your choice: "))
while True:
if user_input in OPERATIONS or user_input == EXIT_KEY:
return user_input
print("Invalid menu option.")
user_input = int(input("Please try again: "))
Now, adding or removing an operation is much more trivial. Also, if we want to start to use letters instead of numbers for the menu options, it is quite easy to do as well.
Details
total = total + 1
can be written total += 1
.
Small functions and responsabilities
Instead of having check_solution
called with a count
value and returning the value incremented or not, we could ensure the value just returns a boolean and the calling function is reponsible for using it.
def menu_option(index, count, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
if check_solution(get_user_solution(problem, player), res):
count += 1
return count
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
Now, the same logic actually applies to menu_option
:
def menu_option(index, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
return check_solution(get_user_solution(problem, player), res)
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
...
while option != EXIT_KEY:
total += 1
if menu_option(option, player):
correct += 1
option = get_user_input()
At that stage, I start to feel that the function names could be improved.
$endgroup$
add a comment |
$begingroup$
Code organisation
Having many functions defined in the middle of the core loop make things very hard to understand. It would be much easier and much more conventional to define all functions before the loop. One of the issues you might encounter is that functions will rely on variables they know nothing about such as players
. A simple solution could be to add these as parameters.
Also, main
is a very confusing name for a function which is not the first thing launched. I guess play_turn
would be a better name.
At this stage, we'd have something like:
Disclaimer: I've changed various details (removed fstrings because I don't have Python 3.6 available, etc)
import random, time
SIMU = True # hack for testing purposes
def display_intro():
title = "** A Simple Math Quiz **"
print("*" * len(title))
print(title)
print("*" * len(title))
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
print(menu_list[0])
print(menu_list[1])
print(menu_list[2])
print(menu_list[3])
print(menu_list[4])
def display_separator():
print("-" * 24)
def get_user_input():
user_input = int(input("Enter your choice: "))
while user_input > 5 or user_input <= 0:
print("Invalid menu option.")
user_input = int(input("Please try again: "))
else:
return user_input
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
return count
else:
print("Incorrect.")
return count
def get_user_solution(problem, player):
if not SIMU:
time.sleep(random.randint(1,8))
then = time.time()
print(problem, end="")
result = int(input(" = "))
reaction_time = time.time()-then
print(" ms")
player["points"] += reaction_time
return result
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
problem = str(number_one) + " + " + str(number_two)
solution = number_one + number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 2:
problem = str(number_one) + " - " + str(number_two)
solution = number_one - number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 3:
problem = str(number_one) + " * " + str(number_two)
solution = number_one * number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
else:
problem = str(number_one) + " // " + str(number_two)
solution = number_one // number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
def display_result(total, correct):
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
if total == 0:
percentage = 0
print("You answered", total, "questions with", correct, "correct.")
print("Your score is ", percentage, "%. Thank you.", sep = "")
winner = min(players,key=lambda a: a['points'])['name']
print("The winner is...")
time.sleep(0.5)
print("{winner}!")
def play_turn(player):
if SIMU:
print("It is now 's turn.nBe ready.")
else:
input("It is now 's turn.nPress enter when ready.")
display_intro()
display_menu()
display_separator()
option = get_user_input()
total = 0
correct = 0
while option != 5:
total = total + 1
correct = menu_option(option, correct, player)
option = get_user_input()
print("Exit the quiz.")
display_separator()
display_result(total, correct)
def play_round(players):
print("-"*20 + "nIt is now round !n" + "-"*20)
for player in players:
play_turn(player)
def play_game():
if SIMU:
players = [{'name': name, 'points': 0} for name in ('foo', 'bar', 'foobar')]
nb_rounds = 1
else:
players = [{
'name':input("Player: "),
'points':0
} for i in range(int(input("How many players? ")))]
nb_rounds = int(input("How many rounds? "))
for i in range(0, nb_rounds):
play_round(players)
play_game()
Now, it it easier to improve these functions.
Improve display_intro
We could compute the length and the border string in a single place:
def display_intro():
title = "** A Simple Math Quiz **"
border = "*" * len(title)
print(border)
print(title)
print(border)
Improve display_menu
It is a good idea to have the menu in a proper data structure. It would be even better too iterate over it instead of using indices: it would be more concise, more efficient and easier to maintain (you do not have to remember to add/remove lines when you add/remove elements from the menu).
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
for menu_item in menu_list:
print(menu_item)
Also, we could have the numbers generated automatically but we'll come back to this.
Improve check_solution
You have the same thing returned in both branches. You could have it in a single place:
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
else:
print("Incorrect.")
return count
Improving display_result
You could use elif
at the beginning because the 2 cases are mutually exclusive:
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
elif total == 0:
percentage = 0
However, I'd rather have this written:
if total == 0:
percentage = 0
else:
result = correct / total
percentage = round((result * 100), 2)
Improve menu_option
The main thing one can see is that it contains highly duplicated code.
Let's extract out the common part to make things clearer:
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
op_str = " + "
res = number_one + number_two
elif index is 2:
op_str = " - "
res = number_one - number_two
elif index is 3:
op_str = " * "
res = number_one * number_two
else:
op_str = " // "
res = number_one // number_two
problem = str(number_one) + op_str + str(number_two)
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, res, count)
return count
You use is
to compare values. You should actually use ==
at that stage. You'll find more online about the is
operator.
Also, instead of writing that code with multiple conditions, we could try to fit the relevant data in a proper data structure, for instance a dictionnary mapping indices to information about the operation to be performed. The operator
module will be helpful here (you could also use lambda functions).
You'd have something like:
import operator
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
operations = {
1: (" + ", operator.add),
2: (" - ", operator.sub),
3: (" * ", operator.mul),
4: (" // ", operator.floordiv),
}
op_str, op_func = operations[index]
res = op_func(number_one, number_two)
problem = str(number_one) + op_str + str(number_two)
count = check_solution(get_user_solution(problem, player), res, count)
return count
Now, we can think about the real meaning of the keys of that dict: they correspond to the user inputs we display in display_menu
.
If we want to add or remove a supported mathematical operation, it is very hard to get it right. We should try to use a Single Source of Truth.
Then we also realise that the same information is also used in get_user_input
along with the input to exit. It would make sense to get rid of that magic number which appears also in play_turn
.
We could write something like:
OPERATIONS = {
1: (" + ", "Addition", operator.add),
2: (" - ", "Substraction", operator.sub),
3: (" * ", "Multiplication", operator.mul),
4: (" // ", "Integer Division", operator.floordiv),
}
EXIT_KEY = 5
def display_menu():
for k, v in OPERATIONS.items():
op_str, op_name, op_func = v
print(str(k) + ". " + op_name)
print(str(EXIT_KEY) + ". Exit")
def get_user_input():
user_input = int(input("Enter your choice: "))
while True:
if user_input in OPERATIONS or user_input == EXIT_KEY:
return user_input
print("Invalid menu option.")
user_input = int(input("Please try again: "))
Now, adding or removing an operation is much more trivial. Also, if we want to start to use letters instead of numbers for the menu options, it is quite easy to do as well.
Details
total = total + 1
can be written total += 1
.
Small functions and responsabilities
Instead of having check_solution
called with a count
value and returning the value incremented or not, we could ensure the value just returns a boolean and the calling function is reponsible for using it.
def menu_option(index, count, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
if check_solution(get_user_solution(problem, player), res):
count += 1
return count
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
Now, the same logic actually applies to menu_option
:
def menu_option(index, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
return check_solution(get_user_solution(problem, player), res)
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
...
while option != EXIT_KEY:
total += 1
if menu_option(option, player):
correct += 1
option = get_user_input()
At that stage, I start to feel that the function names could be improved.
$endgroup$
add a comment |
$begingroup$
Code organisation
Having many functions defined in the middle of the core loop make things very hard to understand. It would be much easier and much more conventional to define all functions before the loop. One of the issues you might encounter is that functions will rely on variables they know nothing about such as players
. A simple solution could be to add these as parameters.
Also, main
is a very confusing name for a function which is not the first thing launched. I guess play_turn
would be a better name.
At this stage, we'd have something like:
Disclaimer: I've changed various details (removed fstrings because I don't have Python 3.6 available, etc)
import random, time
SIMU = True # hack for testing purposes
def display_intro():
title = "** A Simple Math Quiz **"
print("*" * len(title))
print(title)
print("*" * len(title))
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
print(menu_list[0])
print(menu_list[1])
print(menu_list[2])
print(menu_list[3])
print(menu_list[4])
def display_separator():
print("-" * 24)
def get_user_input():
user_input = int(input("Enter your choice: "))
while user_input > 5 or user_input <= 0:
print("Invalid menu option.")
user_input = int(input("Please try again: "))
else:
return user_input
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
return count
else:
print("Incorrect.")
return count
def get_user_solution(problem, player):
if not SIMU:
time.sleep(random.randint(1,8))
then = time.time()
print(problem, end="")
result = int(input(" = "))
reaction_time = time.time()-then
print(" ms")
player["points"] += reaction_time
return result
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
problem = str(number_one) + " + " + str(number_two)
solution = number_one + number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 2:
problem = str(number_one) + " - " + str(number_two)
solution = number_one - number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 3:
problem = str(number_one) + " * " + str(number_two)
solution = number_one * number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
else:
problem = str(number_one) + " // " + str(number_two)
solution = number_one // number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
def display_result(total, correct):
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
if total == 0:
percentage = 0
print("You answered", total, "questions with", correct, "correct.")
print("Your score is ", percentage, "%. Thank you.", sep = "")
winner = min(players,key=lambda a: a['points'])['name']
print("The winner is...")
time.sleep(0.5)
print("{winner}!")
def play_turn(player):
if SIMU:
print("It is now 's turn.nBe ready.")
else:
input("It is now 's turn.nPress enter when ready.")
display_intro()
display_menu()
display_separator()
option = get_user_input()
total = 0
correct = 0
while option != 5:
total = total + 1
correct = menu_option(option, correct, player)
option = get_user_input()
print("Exit the quiz.")
display_separator()
display_result(total, correct)
def play_round(players):
print("-"*20 + "nIt is now round !n" + "-"*20)
for player in players:
play_turn(player)
def play_game():
if SIMU:
players = [{'name': name, 'points': 0} for name in ('foo', 'bar', 'foobar')]
nb_rounds = 1
else:
players = [{
'name':input("Player: "),
'points':0
} for i in range(int(input("How many players? ")))]
nb_rounds = int(input("How many rounds? "))
for i in range(0, nb_rounds):
play_round(players)
play_game()
Now, it it easier to improve these functions.
Improve display_intro
We could compute the length and the border string in a single place:
def display_intro():
title = "** A Simple Math Quiz **"
border = "*" * len(title)
print(border)
print(title)
print(border)
Improve display_menu
It is a good idea to have the menu in a proper data structure. It would be even better too iterate over it instead of using indices: it would be more concise, more efficient and easier to maintain (you do not have to remember to add/remove lines when you add/remove elements from the menu).
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
for menu_item in menu_list:
print(menu_item)
Also, we could have the numbers generated automatically but we'll come back to this.
Improve check_solution
You have the same thing returned in both branches. You could have it in a single place:
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
else:
print("Incorrect.")
return count
Improving display_result
You could use elif
at the beginning because the 2 cases are mutually exclusive:
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
elif total == 0:
percentage = 0
However, I'd rather have this written:
if total == 0:
percentage = 0
else:
result = correct / total
percentage = round((result * 100), 2)
Improve menu_option
The main thing one can see is that it contains highly duplicated code.
Let's extract out the common part to make things clearer:
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
op_str = " + "
res = number_one + number_two
elif index is 2:
op_str = " - "
res = number_one - number_two
elif index is 3:
op_str = " * "
res = number_one * number_two
else:
op_str = " // "
res = number_one // number_two
problem = str(number_one) + op_str + str(number_two)
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, res, count)
return count
You use is
to compare values. You should actually use ==
at that stage. You'll find more online about the is
operator.
Also, instead of writing that code with multiple conditions, we could try to fit the relevant data in a proper data structure, for instance a dictionnary mapping indices to information about the operation to be performed. The operator
module will be helpful here (you could also use lambda functions).
You'd have something like:
import operator
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
operations = {
1: (" + ", operator.add),
2: (" - ", operator.sub),
3: (" * ", operator.mul),
4: (" // ", operator.floordiv),
}
op_str, op_func = operations[index]
res = op_func(number_one, number_two)
problem = str(number_one) + op_str + str(number_two)
count = check_solution(get_user_solution(problem, player), res, count)
return count
Now, we can think about the real meaning of the keys of that dict: they correspond to the user inputs we display in display_menu
.
If we want to add or remove a supported mathematical operation, it is very hard to get it right. We should try to use a Single Source of Truth.
Then we also realise that the same information is also used in get_user_input
along with the input to exit. It would make sense to get rid of that magic number which appears also in play_turn
.
We could write something like:
OPERATIONS = {
1: (" + ", "Addition", operator.add),
2: (" - ", "Substraction", operator.sub),
3: (" * ", "Multiplication", operator.mul),
4: (" // ", "Integer Division", operator.floordiv),
}
EXIT_KEY = 5
def display_menu():
for k, v in OPERATIONS.items():
op_str, op_name, op_func = v
print(str(k) + ". " + op_name)
print(str(EXIT_KEY) + ". Exit")
def get_user_input():
user_input = int(input("Enter your choice: "))
while True:
if user_input in OPERATIONS or user_input == EXIT_KEY:
return user_input
print("Invalid menu option.")
user_input = int(input("Please try again: "))
Now, adding or removing an operation is much more trivial. Also, if we want to start to use letters instead of numbers for the menu options, it is quite easy to do as well.
Details
total = total + 1
can be written total += 1
.
Small functions and responsabilities
Instead of having check_solution
called with a count
value and returning the value incremented or not, we could ensure the value just returns a boolean and the calling function is reponsible for using it.
def menu_option(index, count, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
if check_solution(get_user_solution(problem, player), res):
count += 1
return count
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
Now, the same logic actually applies to menu_option
:
def menu_option(index, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
return check_solution(get_user_solution(problem, player), res)
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
...
while option != EXIT_KEY:
total += 1
if menu_option(option, player):
correct += 1
option = get_user_input()
At that stage, I start to feel that the function names could be improved.
$endgroup$
Code organisation
Having many functions defined in the middle of the core loop make things very hard to understand. It would be much easier and much more conventional to define all functions before the loop. One of the issues you might encounter is that functions will rely on variables they know nothing about such as players
. A simple solution could be to add these as parameters.
Also, main
is a very confusing name for a function which is not the first thing launched. I guess play_turn
would be a better name.
At this stage, we'd have something like:
Disclaimer: I've changed various details (removed fstrings because I don't have Python 3.6 available, etc)
import random, time
SIMU = True # hack for testing purposes
def display_intro():
title = "** A Simple Math Quiz **"
print("*" * len(title))
print(title)
print("*" * len(title))
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
print(menu_list[0])
print(menu_list[1])
print(menu_list[2])
print(menu_list[3])
print(menu_list[4])
def display_separator():
print("-" * 24)
def get_user_input():
user_input = int(input("Enter your choice: "))
while user_input > 5 or user_input <= 0:
print("Invalid menu option.")
user_input = int(input("Please try again: "))
else:
return user_input
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
return count
else:
print("Incorrect.")
return count
def get_user_solution(problem, player):
if not SIMU:
time.sleep(random.randint(1,8))
then = time.time()
print(problem, end="")
result = int(input(" = "))
reaction_time = time.time()-then
print(" ms")
player["points"] += reaction_time
return result
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
problem = str(number_one) + " + " + str(number_two)
solution = number_one + number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 2:
problem = str(number_one) + " - " + str(number_two)
solution = number_one - number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
elif index is 3:
problem = str(number_one) + " * " + str(number_two)
solution = number_one * number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
else:
problem = str(number_one) + " // " + str(number_two)
solution = number_one // number_two
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, solution, count)
return count
def display_result(total, correct):
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
if total == 0:
percentage = 0
print("You answered", total, "questions with", correct, "correct.")
print("Your score is ", percentage, "%. Thank you.", sep = "")
winner = min(players,key=lambda a: a['points'])['name']
print("The winner is...")
time.sleep(0.5)
print("{winner}!")
def play_turn(player):
if SIMU:
print("It is now 's turn.nBe ready.")
else:
input("It is now 's turn.nPress enter when ready.")
display_intro()
display_menu()
display_separator()
option = get_user_input()
total = 0
correct = 0
while option != 5:
total = total + 1
correct = menu_option(option, correct, player)
option = get_user_input()
print("Exit the quiz.")
display_separator()
display_result(total, correct)
def play_round(players):
print("-"*20 + "nIt is now round !n" + "-"*20)
for player in players:
play_turn(player)
def play_game():
if SIMU:
players = [{'name': name, 'points': 0} for name in ('foo', 'bar', 'foobar')]
nb_rounds = 1
else:
players = [{
'name':input("Player: "),
'points':0
} for i in range(int(input("How many players? ")))]
nb_rounds = int(input("How many rounds? "))
for i in range(0, nb_rounds):
play_round(players)
play_game()
Now, it it easier to improve these functions.
Improve display_intro
We could compute the length and the border string in a single place:
def display_intro():
title = "** A Simple Math Quiz **"
border = "*" * len(title)
print(border)
print(title)
print(border)
Improve display_menu
It is a good idea to have the menu in a proper data structure. It would be even better too iterate over it instead of using indices: it would be more concise, more efficient and easier to maintain (you do not have to remember to add/remove lines when you add/remove elements from the menu).
def display_menu():
menu_list = ["1. Addition", "2. Subtraction", "3. Multiplication", "4. Integer Division", "5. Exit"]
for menu_item in menu_list:
print(menu_item)
Also, we could have the numbers generated automatically but we'll come back to this.
Improve check_solution
You have the same thing returned in both branches. You could have it in a single place:
def check_solution(user_solution, solution, count):
if user_solution == solution:
count = count + 1
print("Correct.")
else:
print("Incorrect.")
return count
Improving display_result
You could use elif
at the beginning because the 2 cases are mutually exclusive:
if total > 0:
result = correct / total
percentage = round((result * 100), 2)
elif total == 0:
percentage = 0
However, I'd rather have this written:
if total == 0:
percentage = 0
else:
result = correct / total
percentage = round((result * 100), 2)
Improve menu_option
The main thing one can see is that it contains highly duplicated code.
Let's extract out the common part to make things clearer:
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
if index is 1:
op_str = " + "
res = number_one + number_two
elif index is 2:
op_str = " - "
res = number_one - number_two
elif index is 3:
op_str = " * "
res = number_one * number_two
else:
op_str = " // "
res = number_one // number_two
problem = str(number_one) + op_str + str(number_two)
user_solution = get_user_solution(problem, player)
count = check_solution(user_solution, res, count)
return count
You use is
to compare values. You should actually use ==
at that stage. You'll find more online about the is
operator.
Also, instead of writing that code with multiple conditions, we could try to fit the relevant data in a proper data structure, for instance a dictionnary mapping indices to information about the operation to be performed. The operator
module will be helpful here (you could also use lambda functions).
You'd have something like:
import operator
def menu_option(index, count, player):
number_one = random.randrange(1, 21)
number_two = random.randrange(1, 21)
operations = {
1: (" + ", operator.add),
2: (" - ", operator.sub),
3: (" * ", operator.mul),
4: (" // ", operator.floordiv),
}
op_str, op_func = operations[index]
res = op_func(number_one, number_two)
problem = str(number_one) + op_str + str(number_two)
count = check_solution(get_user_solution(problem, player), res, count)
return count
Now, we can think about the real meaning of the keys of that dict: they correspond to the user inputs we display in display_menu
.
If we want to add or remove a supported mathematical operation, it is very hard to get it right. We should try to use a Single Source of Truth.
Then we also realise that the same information is also used in get_user_input
along with the input to exit. It would make sense to get rid of that magic number which appears also in play_turn
.
We could write something like:
OPERATIONS = {
1: (" + ", "Addition", operator.add),
2: (" - ", "Substraction", operator.sub),
3: (" * ", "Multiplication", operator.mul),
4: (" // ", "Integer Division", operator.floordiv),
}
EXIT_KEY = 5
def display_menu():
for k, v in OPERATIONS.items():
op_str, op_name, op_func = v
print(str(k) + ". " + op_name)
print(str(EXIT_KEY) + ". Exit")
def get_user_input():
user_input = int(input("Enter your choice: "))
while True:
if user_input in OPERATIONS or user_input == EXIT_KEY:
return user_input
print("Invalid menu option.")
user_input = int(input("Please try again: "))
Now, adding or removing an operation is much more trivial. Also, if we want to start to use letters instead of numbers for the menu options, it is quite easy to do as well.
Details
total = total + 1
can be written total += 1
.
Small functions and responsabilities
Instead of having check_solution
called with a count
value and returning the value incremented or not, we could ensure the value just returns a boolean and the calling function is reponsible for using it.
def menu_option(index, count, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
if check_solution(get_user_solution(problem, player), res):
count += 1
return count
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
Now, the same logic actually applies to menu_option
:
def menu_option(index, player):
n1 = random.randrange(1, 21)
n2 = random.randrange(1, 21)
op_str, op_name, op_func = OPERATIONS[index]
res = op_func(n1, n2)
problem = str(n1) + op_str + str(n2)
return check_solution(get_user_solution(problem, player), res)
def check_solution(user_solution, solution):
correct = user_solution == solution
print("Correct." if correct else "Incorrect.")
return correct
...
while option != EXIT_KEY:
total += 1
if menu_option(option, player):
correct += 1
option = get_user_input()
At that stage, I start to feel that the function names could be improved.
edited Feb 7 at 22:39
answered Feb 5 at 0:59
JosayJosay
25.8k14087
25.8k14087
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212884%2fexpired%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
$begingroup$
(My pet peeve: you present uncommented/undocumented code.)
I'm experiencing some issues
please describe these, and why you have trouble resolving them.$endgroup$
– greybeard
Feb 4 at 23:36
$begingroup$
Welcome to Code Review! When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 4 at 23:36
$begingroup$
Welcore here! I have the feeling the code is not formatted properly. I highly recommend using the syntax: <triple backticks><newline><your code><newline><triple backticks> to format bug chunks of code (backtick is `)
$endgroup$
– Josay
Feb 5 at 0:15
$begingroup$
Actually, the code may be properly formated but is just confusing.
$endgroup$
– Josay
Feb 5 at 0:16
$begingroup$
@Josay I recommend reading the Help center page for markdown formatting of code. The OP has used the standard four spaces for indentation. Code fences (as you described with three backticks) are supported as well - click the show more link to read more about that section.
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Feb 5 at 0:36