Skip to content

Sno Ochoa C23 Snowman Feedback #34

Open
mikellewade wants to merge 2 commits intoAda-C23:mainfrom
ra-choa:main
Open

Sno Ochoa C23 Snowman Feedback #34
mikellewade wants to merge 2 commits intoAda-C23:mainfrom
ra-choa:main

Conversation

@mikellewade
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Author

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

Nice work, Sno! I left a comment about your implementation missing one of the requirements for the function. Please be sure to respond to the comment. If you have any questions about the feedback I left let me know!

game.py Outdated
correct_letter_guess_statuses[letter_typed] = True
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
if is_word_guessed(snowman_word, correct_letter_guess_statuses) is True:
# if snowman_word == generate_word_progress_string(snowman_word, correct_letter_guess_statuses):
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comments like these usually you want to delete. We usually want to keep comments that we that we feel give some vital insight to our code. This seems like it was maybe an other rendition of your code so I would be okay with deleting this comment.

if is_word_guessed(snowman_word, correct_letter_guess_statuses) is True:
# if snowman_word == generate_word_progress_string(snowman_word, correct_letter_guess_statuses):
print("Congratulations, you win!")
return
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nice use of the return keyword. This allows for us to exit not only the while loop early if this condition is met but also the function as a whole.

game.py Outdated
wrong_guesses_list = []
correct_letter_guess_statuses = build_letter_status_dict(snowman_word)

while True:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

While there is nothing wrong with this while loop functionally, we could also change it to something more explicit to more readily display the purpose of this loop! It could be something like this:

while len(wrong_guesses_list) >= SNOWMAN_MAX_WRONG_GUESSES:

This readily lets other developers know that this loop is dependent on the guesses the a user is making.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This makes, more sense in my brain as well so I updated your suggestion! 😸

Comment on lines +30 to +31
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
if is_word_guessed(snowman_word, correct_letter_guess_statuses) is True:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For readability I would add a space between lines like these. It helps break up your code into sections and groups things based on their responsibility.

Suggested change
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
if is_word_guessed(snowman_word, correct_letter_guess_statuses) is True:
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
if is_word_guessed(snowman_word, correct_letter_guess_statuses) is True:

# if snowman_word == generate_word_progress_string(snowman_word, correct_letter_guess_statuses):
print("Congratulations, you win!")
return
else:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Your code is missing an implementation of one of the requirements. Your snowman function should display the list of incorrect guesses. @ra-choa please respond to this comment with how you would implement this feature.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you Mikelle, I missed this because I only looked at the readme and forgot to refer back to the Learn page. Updated accordingly here 🫡

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