Possum - Mumina A.#20
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
| replace "pass" below with your own code | ||
| It should print 'Congratulations, you win!' | ||
| If the player wins and, | ||
| If the player wins and, |
There was a problem hiding this comment.
Not a big deal in this project, but in general, try to minimize edits to parts of a file outside the actual logic changes. In a code review on a professional team, stray edits throughout a file can distract from the core of what the team needs to review.
Cleaning up spaces and formatting is helpful, but often, teams will review formatting fixes separately from logic fixes. I.e., for these formatting fixes, we would hold them back until our logic changes were signed off and integrated into the project. Then we'd submit a separate change that solely focuses on formatting fixes.
| correct_letter_guess_statuses = build_letter_status_dict(snowman_word) | ||
| wrong_guesses_list = [] |
There was a problem hiding this comment.
👍 These are the two pieces of data we need to make use of our other helper functions for tracking correct and incorrect guesses. Tracking the letter status involves using a dictionary with a particular structure, which our build_letter_status_dict helper is able to calculate from the random word.
| correct_letter_guess_statuses = build_letter_status_dict(snowman_word) | ||
| wrong_guesses_list = [] | ||
|
|
||
| while True: |
There was a problem hiding this comment.
Personally, I'm a fan of infinite loops as a control mechanism, especially when the loop condition would be complicated. And when the loop ends, we don't know why without checking again. Even if we pare it down to just a portion (say, the player ran out of guesses), we still need extra logic to check for the win case. And why should that portion of the condition be the one elevated to the loop control?
Just keep in mind this isn't a popular view on every team, and that many will prefer an explicit loop control condition as it ensures there's one place where conditions that should terminate the loop are all checked. One way to address this is to move a complicated check into a helper function, which could express this line as something like:
while should_keep_playing(correct_letter_guess_statuses, wrong_guesses_list):assuming should_keep_playing is a helper that does the checks for whether the word was guessed or the number of guesses are exceeded.
| wrong_guesses_list = [] | ||
|
|
||
| while True: | ||
| user_input = get_letter_from_user(correct_letter_guess_statuses, wrong_guesses_list) |
There was a problem hiding this comment.
Since we don't display the word status until after the guess, the player doesn't get to see the length of the word before guessing. This wasn't a requirement, but it would be nice to have from a gameplay perspective.
| while True: | ||
| user_input = get_letter_from_user(correct_letter_guess_statuses, wrong_guesses_list) | ||
|
|
||
| if user_input in correct_letter_guess_statuses: |
There was a problem hiding this comment.
👍 Nice decision to check whether the letter was in the word by using the status dict rather than the word itself. Checking for the presence of a value in a dict is more efficient than checking in a str, even when both use the in operator. It won't matter much in this small program, but it's helpful to start thinking about these differences and biasing our choices towards efficiency!
|
|
||
| if user_input in correct_letter_guess_statuses: | ||
| # Only flip to True if it wasn't already guessed correctly | ||
| if not correct_letter_guess_statuses[user_input]: |
There was a problem hiding this comment.
Note that we don't need to check whether the guess was already set to True before setting it again. In the first place, it wouldn't cause a problem to overwrite the value. But more importantly, get_letter_from_user will not return until a new letter has been guessed. Therefore, any correct letter must be a new correct letter, and therefore we need to update the status.
| # Only flip to True if it wasn't already guessed correctly | ||
| if not correct_letter_guess_statuses[user_input]: | ||
| correct_letter_guess_statuses[user_input] = True | ||
| print_word_progress_string(snowman_word, correct_letter_guess_statuses) |
There was a problem hiding this comment.
Consider printing status information, including the word progress (shows correct letters), the incorrect letters, and the snowman graphic, each time through the loop regardless of whether the previous letter was correct or incorrect. Having a consistent set of information can make it easier for the player to understand their situation and can help avoid the issue of information potentially scrolling off the screen.
| print_word_progress_string(snowman_word, correct_letter_guess_statuses) | ||
| else: | ||
| # Only record a wrong guess once | ||
| if user_input not in wrong_guesses_list: |
There was a problem hiding this comment.
As for the correct guesses, due to how get_letter_from_user works, we know that any incorrect letter is a new incorrect letter. So we don't need this check.
| print(f"Congratulations, you win!") | ||
| return # stop game immediately | ||
|
|
||
| if len(wrong_guesses_list) >= SNOWMAN_MAX_WRONG_GUESSES: |
There was a problem hiding this comment.
Theoretically, we only need to check for equality here, as the player can only guess a single letter at a time meaning they should never be able to jump over the max number of wrong guesses. But the greater than check doesn't hurt anything.
There was a problem hiding this comment.
👍 Nice decision to get the count of wrong guesses directly from the length of the wrong guesses list when needed. If we had used a separate variable to track the count, it would be possible for the values to become out of sync, leading to player confusion. Though unlikely in a program this short, in longer programs it can be more difficult to be sure that related values are always updated together. Forgetting to update both in one place can lead to them drifting.
In some languages, getting the length of certain data types is a relatively slow operation. Python is fast for most built-in containers (and strings), so it's fine to call len() as many times as we need to. But we do need to understand the costs of operations in the languages we are working in. In a language where getting the length of a container might be costly (or even impossible), it might be worthwhile (or necessary) to track the count in a separate variable.
2 participants
No description provided.