Skip to content

Snowman Feedback - Joyce Wu#3

Open
yangashley wants to merge 1 commit intoAda-C24:mainfrom
xiwu5:main
Open

Snowman Feedback - Joyce Wu#3
yangashley wants to merge 1 commit intoAda-C24:mainfrom
xiwu5:main

Conversation

@yangashley
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Collaborator Author

@yangashley yangashley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Please review my comments, and let me know if you have any questions.

Comment thread game.py
pass
correct_letter_guess_statuses = build_letter_status_dict(snowman_word)
wrong_guesses_list = []
wrong_guesses_count = 0
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this variable wrong_guess_count? Instead of initializing wrong_guess_count to 0 and needing to manually increment the value on line 50 (where you write wrong_guess_count += 1), we should prefer to use a built-in method to get the length of wrong_guesses_list. Needing to manually increment a value can introduce logical bugs.

Since every wrong guess is appended to wrong_guesses_list, we can feel confident that the length of the list reflects how many wrong guesses have been made.

By using the len method, we use a built-in method and can remove lines 25 and 50. This isn't an endorsement for writing the shortest code possible because there can be times when we need to write more lines to make code more human-readable, but for simple logic like this we should try to avoid needing additional logic when we can use a built-in method.

Comment thread game.py
Comment on lines +29 to +30
# TESTING/DEBUGGING
#print(f"TESTING: {snowman_word}")
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# TESTING/DEBUGGING
#print(f"TESTING: {snowman_word}")

A note about comments: Folks might find it useful to leave comments that explain what different lines/sections of code do in the beginning of their coding career. However, when we write self documenting code (code with descriptively named variables and function names) that is concise and easy to follow, then there should be no need for comments. For example, if a function is named get_letter_from_user, I can deduce what the function should do without needing to read a comment that explains what it does.

Additionally, any debugging comments should be removed because they can introduce confusion to other devs reading your code. I might ask myself, should we leave line 30 in? If line 30 were to accidentally get uncommented then this code would execute when maybe it shouldn't have. Here, we have a fairly innocuous line of code, but imagine line 30 was a time consuming call to a database to retrieve some data. If it was uncommented and ran, then we'd be introducing latency into our program.

Finally, comments add more lines of code in a codebase. More lines of code make a codebase more challenging to maintain and update and also create more areas where a bug can accidentally get introduced.

Teams have differing opinions about comments. Some teams may be ok with some comments here and there and some teams prefer no comments unless absolutely necessary. You’ll find out how your team feels when you get there!

My opinion, when it comes to reviewing your work, I prefer to not have comments because if the code is concise and clear then I should be able to understand it.

Comment thread game.py
Comment on lines +32 to +33
while (wrong_guesses_count < SNOWMAN_MAX_WRONG_GUESSES
and not is_word_guessed(snowman_word, correct_letter_guess_statuses)):
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do use a line break so that the code on lines 32 and 33 do not exceed the max line length as described by PEP8.

However, even though the code follows the style guide, it is still a little difficult to read. A signal to me that something might be tough to read is if I have to slow down and interpret what's going on, which I had to do here.

Since we write code for other humans to read, we should try to make our code easy to follow. Here, I might introduce a descriptively named variable and use it as the condition for the while loop.

is_player_still_guessing = wrong_guesses_count < SNOWMAN_MAX_WRONG_GUESSES
is_game_still_in_play = not is_word_guessed(snowman_word, correct_letter_guess_statuses)

while is_player_still_guessing and is_game_still_in_play:
    # rest of your logic here

This refactor adds a couple more lines of code, which is a tradeoff, but it does make the condition on line 32-33 easier to understand. My variable names might not be what you would name them, but this just gives you an idea of how we can make a lengthy condition more readable so feel free to come up with your own ideas for how to name the variables!

Comment thread game.py
if user_letter_guess in correct_letter_guess_statuses:
correct_letter_guess_statuses[user_letter_guess] = True
print(f"Good guess! The letter '{user_letter_guess}' is in the word.")
#print_word_progress_string(snowman_word, correct_letter_guess_statuses)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging print statements when you're done debugging and your project is ready for submission

Suggested change
#print_word_progress_string(snowman_word, correct_letter_guess_statuses)

Comment thread game.py
wrong_guesses_list.append(user_letter_guess)
wrong_guesses_count += 1
print(f"Sorry, the letter '{user_letter_guess}' is not in the word.")
#print_word_progress_string(snowman_word, correct_letter_guess_statuses)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging print statements when you're done debugging and your project is ready for submission

Suggested change
#print_word_progress_string(snowman_word, correct_letter_guess_statuses)

Comment thread game.py

else:
wrong_guesses_list.append(user_letter_guess)
wrong_guesses_count += 1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wrong_guesses_count += 1

See comment above about using the length of wrong_guesses_list, instead of introducing an extra variable wrong_guesses_count.

Comment thread game.py
print("Congratulations, you win!")
print(f"You guessed the word {snowman_word}!")
else:
print_snowman_graphic(wrong_guesses_count)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print_snowman_graphic(wrong_guesses_count)
print_snowman_graphic(len(wrong_guesses_list))

When the unnecessary variable wrong_guesses_count is removed, then you can find the length of the length of wrong_guesses_list and pass that value to print_snowman_graphic

Comment thread game.py
Comment on lines +32 to +33
while (wrong_guesses_count < SNOWMAN_MAX_WRONG_GUESSES
and not is_word_guessed(snowman_word, correct_letter_guess_statuses)):
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice job including the 2 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants