-
Notifications
You must be signed in to change notification settings - Fork 89
Snowman Feedback - Kat Bello #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
apradoada
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kat,
Welcome to your first Pull Request (PR)! We will talk more about pull requests during Unit 1, but they are a great tool we have on Github to easily view and provide feedback on changes that have been made to a repository. In industry, they are often used as a way for senior developers to provide feedback to junior developers on code they have written before that code gets merged into a deployed project. Here at Ada, we use it to provide feedback on your projects! Today, I've made the PR on your behalf, but in the future, you'll make your own and submit the link to that PR instead of a link to your project.
When it comes to the feedback I give, we use a scale of Red, Yellow, Green. They mean the following:
🟢 Green 🟢: Green projects pass all the tests and your code doesn't include anything that could cause the program to behave unexpectedly. The feedback on a green project is usually more stylistic in nature or geared toward making specific pieces of code more efficient and less repetitive. You are not required to implement the feedback, but you are welcome to if you want to! However, we likely won't have time to go back and assess the changes you have made. More generally, green projects indicate that we feel you have a strong grasp of the concepts covered in the project.
🟡 Yellow 🟡: Yellow projects typically pass all of the tests provided, but do include code that may indicate uncertainty about certain concepts or how the program works as a whole. This may include code logic that causes unexpected behavior in specific situations that aren't covered in our tests. Feedback on a yellow project may be geared more toward helping you understand why a particular piece of code may fail in those circumstances. Implementing this feedback is not required, but it it highly encouraged to help you recognize patterns and implement similar solutions in the future!
🔴 Red 🔴: Red projects typically do not pass all of the tests provided. Writing code that passes all tests is an element of Test Driven Development (TDD) that is often used within the tech industry. Feedback on a red project typically includes the same type of feedback you would see on a green or yellow project along with feedback to help you understand why your code is not passing all the tests and how you could get there. If you receive a red on a project, you will be required to implement the feedback so that all tests are passing! We will then reassess when the feedback has been implemented!
As far as Snowman goes, this project is a 🟢 Green 🟢! Great Job!
The feedback I have left is just a few things to think about, but overall the logic looks good! You are welcome to implement the changes if you would like! If you have any questions about any of the feedback I have provided, feel free to reach out and I am happy to explain further.
|
|
||
| letter_dictionary = build_letter_status_dict(snowman_word) | ||
| wrong_guesses_list = [] | ||
| count = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can sometimes be useful to have a counter to keep track of how many items we have. One thing to remember however is that most collections (lists, sets, dictionaries, etc...) have a built in attribute that keeps track of how many items are in it. To access that attribute, we just have to use the len() function. Because access is so easy, it's good practice to just use the len() function as opposed to duplicating that information with your own variable.
| letter_dictionary = build_letter_status_dict(snowman_word) | ||
| wrong_guesses_list = [] | ||
| count = 0 | ||
| letter_input = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking through the rest of the program to see where else letter_input gets used. It appears that the only place you use the variable is within the body of your while loop. This means that letter_input's scope is limited to the loop. With that in mind, we actually don't need to declare it outside the loop and keeping it on line 34 is sufficient!
| count = 0 | ||
| letter_input = None | ||
|
|
||
| while (count < SNOWMAN_MAX_WRONG_GUESSES and not is_word_guessed(snowman_word, letter_dictionary)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this particular program, we have two potential finish conditions: 1. We find the word, 2. We use up all our guesses. One is a win condition and one is a lose condition.
There are a couple different ways we could handle these two conditions:
- Place both in a compound conditional within the while loop.
- Use one as the while loop conditional and nest the other conditional within the while loop.
You have opted for the former, which Is good, but it does increase a bit of the work that needs to be done outside the loop. This happens for the following reasons:
-
If we check both for each iteration, our loop ends if either the win or lose condition is met. This means that we are going to have to make an additional and unnecessary check after the loop to see which condition was met. If we somehow resolved one win condition inside the loop, we would remove the need for extra checks after the while loop has complete.
-
If we meet one of the conditions inside the loop, we can exit the function early with an explicit
return. This resolves one of our conditions that we no longer have to worry about later on.
Overall your approach works, but it does add a little bit of extra code!
| print_word_progress_string(snowman_word, letter_dictionary) | ||
| print("\n") # Print a blank line for readability | ||
|
|
||
| letter_input = get_letter_from_user(letter_dictionary, wrong_guesses_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it comes to writing Python code, one of the guides you'll see us reference is the PEP 8 Style Guide. This is a guide for best practices when it comes to styling our code. One of the most common things we see is to try and keep individual lines of code under 79 characters. This line currently goes past that limit! It won't cause the code to break or anything, but it is a good thing to keep in mind in terms of readability and best practices!
I currently see two possible fixes here:
-
Shorter and more concise variable names:
- While there is no hard and fast rule on how long variable names should be, we could first try shortening the variable names we use here (
correct_letter_guess_statusesandwrong_guesses_list) as they are on the longer side. That being said, these names match the parameters for the function you use to call them, which is good practice, so we'll need a different approach (but shorter variable names can help with this in other circumstances).
- While there is no hard and fast rule on how long variable names should be, we could first try shortening the variable names we use here (
-
Restyle the Function Call:
- This is a great trick to have up your sleeve when making a function call that either includes multiple parameters or starts to approach that 79 character line limit! We can drop the parameters to the next line(s) to look something like the following:
Suggested changeletter_input = get_letter_from_user(letter_dictionary, wrong_guesses_list) letter_input = get_letter_from_user( letter_dictionary, wrong_guesses_list ) - This is known as Implicit Line Continuation in Python. Whenever we have information nested inside parentheses, brackets or curly braces, we can drop the nested information to new lines without any specific notation!
| if letter_input in letter_dictionary: | ||
| letter_dictionary[letter_input] = True | ||
| if is_word_guessed(snowman_word, letter_dictionary): | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaks can be a great way to pop out of a loop early. We usually want to reserve them for if we have code that still needs to run after the loop. In this case, guessing the word correctly is one of the ways our function ends completely. With this in mind, we could use the more powerful return statement. This goes along with one of my earlier comments about figuring out which ending condition to use as a control for your while loop. If we included our win message before the return, we could remove that as a condition in our loop header as it is currently in three different places. That would also mean that as soon as the user finds a winning word, the function stops and the only thing we have to worry about after the loop is dealing with the lose condition.
| wrong_guesses_list.append(letter_input) | ||
| count += 1 | ||
| if count == SNOWMAN_MAX_WRONG_GUESSES: | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment from above stands here as well. If we streamline this code just a little bit, we can remove a few of the duplicate conditional statements that exist!
|
|
||
| letter_input = get_letter_from_user(letter_dictionary, wrong_guesses_list) | ||
|
|
||
| if letter_input in letter_dictionary: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to highlight a really cool thing that you did here! When we receive a letter from the user, we want to check to see if the letter exists within the word or not. Given the way this program is set up, we actually end up with two different collections that hold all the letters of the word, a string (snowman_word) and a dictionary (letter_dictionary).
While we could check either one for the letter we've received, searching within a dictionary is ever so slightly more efficient than searching through a string (We'll talk more about why in Unit 1), so great choice here!
Snowman Feedback - Kat Bello