Skip to content

Snowman Feedback - Dehui Hu#40

Open
apradoada wants to merge 4 commits intoAda-C23:mainfrom
florasmile:main
Open

Snowman Feedback - Dehui Hu#40
apradoada wants to merge 4 commits intoAda-C23:mainfrom
florasmile:main

Conversation

@apradoada
Copy link
Copy Markdown

Welcome to your first Pull Request (PR), Dehui! In industry, when you want to merge any changes you have made to an existing codebase, a PR is used as a form of code review where a manager or peer can easily see the changes that have been made and review the new code that has been added! We will discuss them further in Unit 1, but they will be the primary method we use to give feedback on projects! Feel free to look through my comments and don't hesitate to reach out if you have any questions!

Copy link
Copy Markdown
Author

@apradoada apradoada left a comment

Choose a reason for hiding this comment

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

Great job, Dehui!

Feel free to look through the feedback I have left here. Overall, the logic is sound, your tests pass and everything looks good! The suggestions I made are purely for adhering to coding conventions you will find in industry! None of them require updates unless you would like to update them.

Two other small general notes:

  1. When it comes to commenting your code, we want to find a balance between too few comments and too many! While it is usually better to have more than you need rather than not enough, having a comment for every line starts to make things a little unreadable. It is usually a better idea to reserve comments for blocks of code as opposed to every single line!

  2. I see there are just a few commits. This is a pretty basic project, so that makes sense. As we do more robust assignments, you will hear us say "commit early and often" during the program. In general, smaller and more commits can be helpful to see the full history of what you have done and go back and figure out where breaks happen! As we do larger projects, feel free to make more commits!

Comment thread game.py Outdated
if user_input in letter_statuses_dict:
letter_statuses_dict[user_input] = True
# check is_word_guessed, if yes, player wins, print message, exit;
if is_word_guessed(snowman_word, letter_statuses_dict):
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 this isn't inherently too big of a deal, it can be a good idea to avoid nested conditionals if it isn't necessary. In this particular instance, checking if the word has been guessed correctly isn't dependent upon whether or not the guessed letter is in the word. If we moved this nested if statement to be a standalone conditional, the code will work the exact same way. In a situation like this where the location of the if statement does not change how the code works, it's generally a good idea to err on the side of un-nested conditionals!

Comment thread game.py Outdated
# check is_word_guessed, if yes, player wins, print message, exit;
if is_word_guessed(snowman_word, letter_statuses_dict):
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.

Using a return statement to end the loop as soon as a win condition is met is such a smart move! I am a huge advocate for ending a function early when it makes sense!

Comment thread game.py
print_snowman_graphic(len(wrong_guesses_list))

# player loses, print message
print(f"Sorry, you lose! The word was {snowman_word}")
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.

You've made a great choice here as well by recognizing that the win condition is handled in the loop so we don't have to worry with any conditionals outside of the loop! Great catch!

Copy link
Copy Markdown

@florasmile florasmile left a comment

Choose a reason for hiding this comment

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

Love all the comments. Accept all feedback from instructor Adrian and will make changes to the nested conditionals to the project and commit again.

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