Skip to content

Snowman Feedback - Brian Martinez#28

Open
apradoada wants to merge 3 commits intoAda-C23:mainfrom
bamart-dev:main
Open

Snowman Feedback - Brian Martinez#28
apradoada wants to merge 3 commits intoAda-C23:mainfrom
bamart-dev:main

Conversation

@apradoada
Copy link
Copy Markdown

Welcome to your first Pull Request (PR), Brian! 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!

…t appropriate win/loss message to console upon game loop completion.
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, Brian!

Feel free to look through the feedback I have left here. Overall, everything looks good and the suggestions I made are purely for adhering to coding conventions you will find in industry or just pointing out a job well done! None of them require updates unless you would like to update them.

Two small general notes:

  1. When it comes to commenting your code, it can be a good idea to include comments for blocks of code to describe what they are doing. You currently don't have any, so in the future, just make sure to include a few! My rule of thumb for comments is when I have a section of my code where the algorithm may or may not be intuitive. A comment can be super useful for others who read your code to quickly pick up what a specific function or code block is doing.

  2. This is a pretty basic project, so it makes sense to not have too many commits. As we do more robust assignments, you will hear us say "commit early and often" during the programming process. 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
Comment on lines +26 to +27
while (not is_word_guessed(snowman_word, correct_letter_guess_statuses)
and len(wrong_guesses_list) != SNOWMAN_MAX_WRONG_GUESSES):
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.

Great job splitting this compound conditional across multiple lines! According to the PEP8 style guidelines that we use here at Ada, single lines of code should not be more than 79 characters. You can use this documentation for reference: https://peps.python.org/pep-0008/#maximum-line-length

Comment thread game.py
Comment on lines +38 to +40
print_snowman_graphic(len(wrong_guesses_list))
print_word_progress_string(snowman_word, correct_letter_guess_statuses)
print(f"Wrong guesses: {wrong_guesses_list}")
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.

Overall this looks good, but when everything does get printed out in the console, it does get to be a little cluttered. When working with console output, it can sometimes be a good idea to add in some line breaks just to make the flow of the game a little more readable and easy to follow!

Comment thread game.py
Comment on lines +42 to +45
if len(wrong_guesses_list) == SNOWMAN_MAX_WRONG_GUESSES:
print(f"Sorry, you lose! The word was {snowman_word}")
else:
print("Congratulations, you win!")
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.

This conditional looks great!

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