Possum - Wenxin L.#17
Conversation
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good overall, but please review my comments, and let me know if you have any questions. Your general approach is fine, but there are a few things I'd like you to revisit.
| snowman_word_status_dict = build_letter_status_dict(snowman_word) | ||
| correct_guesses_list = [] | ||
| wrong_guesses_list = [] | ||
| wrong_guesses = 0 |
There was a problem hiding this comment.
Of these variables, only snowman_word_status_dict and wrong_guesses_list are required. The dictionary returned by build_letter_status_dict has the correct letters as keys, and the value for each key is whether it's been guessed or not. Storing correct guesses in correct_guesses_list duplicates this information.
Similarly, the number of wrong guesses can be obtained by getting the len() of wrong_guesses_list. Minimizing the amount of duplicate data is usually desirable to reduce the likelihood of the data becoming out of sync. For smaller projects like this, this is less of a concern, but as projects grow, it's easier to miss updating dependent information (the wrong count) everywhere the independent information is updated (the wrong list).
| correct_guesses_list = [] | ||
| wrong_guesses_list = [] | ||
| wrong_guesses = 0 | ||
| while wrong_guesses < SNOWMAN_MAX_WRONG_GUESSES and not is_word_guessed(snowman_word, snowman_word_status_dict): |
There was a problem hiding this comment.
👍 Nice inclusion of both key checks for whether the game is still in progress. The game continues as long as the allowed number of wrong guesses haven't been used up, and the word hasn't yet been guessed. We had to be careful to express this with "as long as" phrasing, which is a tricky part of working with while loops since people often think of doing something "until" a condition is met rather than "as long as" a condition continues to hold.
There was a problem hiding this comment.
The main guide describing how to format our Python code (PEP8) suggests a maximum line length of 79 characters. It can be OK to go a little over, but if it gets too much longer, we should wrap our lines. PEP8 suggests doing this by surrounding our long expressions in parentheses, which lets us wrap lines anywhere we like.
Note as well that this is the only place the value in wrong_guesses is used. If we replace it with a len() check on the wrong guess list, we could remove that variable (and any other code using it) entirely.
while (len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES
and not is_word_guessed(snowman_word, correct_letter_guess_statuses)):There was a problem hiding this comment.
Alternatively, we could shorten this condition to focus solely on the number of guesses, and have a check within the loop for whether the word has been guessed.
while len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES:
# later
if is_word_guessed(snowman_word, correct_letter_guess_statuses):
print("Congratulations, you win!")
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
returnThere was a problem hiding this comment.
Made it into two lines with \ , is there other way to do it?
There was a problem hiding this comment.
Note my second comment in this thread, which uses () to surround the expression, allowing us to wrap the line anywhere within the expression.
PEP8 recommends this style of line wrapping over the use of \.
I also indented the wrapped portion deeper (8 spaces) than the loop block, to help visually separate the wrapped line from the block logic.
while (len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES
and not is_word_guessed(snowman_word, correct_letter_guess_statuses)):```| correct_guesses_list.append(letter) | ||
| snowman_word_status_dict[letter] = True |
There was a problem hiding this comment.
The snowman_word_status_dict is the only value we need to update to track the correct guesses. We can remove correct_guesses_list from the program and any code that works with it.
| print_snowman_graphic(len(wrong_guesses_list)) | ||
| print(f"Wrong guesses: {wrong_guesses_list}") | ||
|
|
||
| if is_word_guessed(snowman_word, snowman_word_status_dict): |
There was a problem hiding this comment.
👍 Our loop terminates either because the number of wrong guesses was used up, or because the word has been guessed. As a result, we do need to double check whether the game was won or lost. With this structure, it would be just as valid to check whether we were out of guesses, meaning the player lost, otherwise they won.
Another alternative would be to drive the main loop using only one of the conditions (either there are still wrong guesses available, OR that the word hasn't yet been guessed). Then within the loop, we could check for the other condition and handle it directly. For example, if we ran the main loop based on the number of wrong guesses alone, we could have an if check within the loop checking for the word being guessed. If we find that the word has been guessed, we could print out the winning message, then return from the entire function. This would then mean that if we exited the loop normally, it must have been because the player lost, so we could print the lose message without needing to double check whether the player won or lost.
| wrong_guesses_list.append(letter) | ||
| wrong_guesses += 1 | ||
|
|
||
| print_snowman_graphic(len(wrong_guesses_list)) |
There was a problem hiding this comment.
👍 You got the wrong guess count from the length of the wrong guess list. This guarantees that the two quantities remain in sync and reduces the need for a separate variable to track that quantity.
| wrong_guesses += 1 | ||
|
|
||
| print_snowman_graphic(len(wrong_guesses_list)) | ||
| print(f"Wrong guesses: {wrong_guesses_list}") |
There was a problem hiding this comment.
Python is able to print out a list of strings, which lets us show the wrong guesses to the player. However, we might consider writing our own function to customize the string we build (try looking into the join method of the string type) since the default output is somewhat "programmer-y." A typical player doesn't need to see the surrounding [] nor the '' around each character.
| wrong_guesses_list = [] | ||
| wrong_guesses = 0 | ||
| while wrong_guesses < SNOWMAN_MAX_WRONG_GUESSES and not is_word_guessed(snowman_word, snowman_word_status_dict): | ||
| letter = get_letter_from_user(wrong_guesses_list, correct_guesses_list) |
There was a problem hiding this comment.
👀 When calling a function, Python only checks that we've passed the correct number of arguments, but it doesn't check the types and it can tell the intended use. The get_letter_from_user function is implemented to expect the first parameter to be a dict with the correct guess information (such as we get as a result from calling build_letter_status_dict), and the second parameter to be the list of wrong guesses.
The code appears to function, as long as we don't repeat a wrong guess. Doing so tries to access the correct guess dict using letter keys, but we've passed in the wrong guess list, which can only be indexed by position, resulting in a crash. If we're careful to avoid repeating a guess, we can see that the message reported is backwards (it prints that we guessed an incorrect letter when it's correct and vice versa).
There was a problem hiding this comment.
👀 Throughout the game, we never see the word progress shown (print_word_progress_string). Without this information, the game isn't very playable for an actual player, even though the tests all pass. We should make sure to display that key game information to the player.
anselrognlie
left a comment
There was a problem hiding this comment.
Thanks for the updates. This all looks good!
| print(f"Wrong guesses: {wrong_guesses_list}") | ||
| print(f"Wrong guesses: {", ".join(wrong_guesses_list)}") | ||
|
|
||
| print_word_progress_string(snowman_word, snowman_word_status_dict) |
There was a problem hiding this comment.
👍 This lets the player see their progress on the word (including the number and order of any correct letters).
|
|
||
| print_snowman_graphic(len(wrong_guesses_list)) | ||
| print(f"Wrong guesses: {wrong_guesses_list}") | ||
| print(f"Wrong guesses: {", ".join(wrong_guesses_list)}") |
There was a problem hiding this comment.
👍 Nice update to the display of the wrong letters. This removes the extra "noise" python includes when printing a raw list.
| while len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES \ | ||
| and not is_word_guessed(snowman_word, snowman_word_status_dict): | ||
| #get a valid letter from the user | ||
| letter = get_letter_from_user(snowman_word_status_dict, wrong_guesses_list) |
There was a problem hiding this comment.
👍 The call to this function now passes arguments of the correct type in the correct order.
| correct_guesses_list = [] | ||
| wrong_guesses_list = [] | ||
| wrong_guesses = 0 | ||
| while wrong_guesses < SNOWMAN_MAX_WRONG_GUESSES and not is_word_guessed(snowman_word, snowman_word_status_dict): |
There was a problem hiding this comment.
Note my second comment in this thread, which uses () to surround the expression, allowing us to wrap the line anywhere within the expression.
PEP8 recommends this style of line wrapping over the use of \.
I also indented the wrapped portion deeper (8 spaces) than the loop block, to help visually separate the wrapped line from the block logic.
while (len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES
and not is_word_guessed(snowman_word, correct_letter_guess_statuses)):```
No description provided.