Snowman Feedback - Gina Song

[apradoada]

Pull Request to add Feedback for Snowman - Gina Song!

[apradoada]

Hi Gina,
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 🟡 Yellow 🟡! Great Job! Almost there!

The feedback I have left is just a few things to think about, with a focus on some logical errors that cause the program to pass tests but run contrary to what is expected. Implementing the changes is not required, but I do encourage you to do so if you have time. Specifically, I would recommend removing the duplicate dictionary. 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.

-Adrian

[apradoada]

These two lines seem to do the exact same thing, and as I was going through the rest of the code, it will cause issues given how it is executed. I understand why you might be tempted to have two snowman dictionaries to hold separate information, but in this particular case, one is more than enough!

[gina0song]

Thank you for pointing this out! I will remove snowman_word_dic and now only use correct_letter_guess_statuses to track guessed letters:correct_letter_guess_statuses = build_letter_status_dict(snowman_word)

[apradoada]

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.

[gina0song]

Good point! I will remove wrong_guesses_count and use len(wrong_guesses_list) to track:wrong_guesses_list = set()

[apradoada]

This is one place where that len() function can really come in handy! I would try something along the lines of:

Suggested change

	while wrong_guesses_count < SNOWMAN_MAX_WRONG_GUESSES:
	while len(wrong_guesses_list) < SNOWMAN_MAX_WRONG_GUESSES:

[apradoada]

This is where the duplicate dictionaries from above start to cause an issue. You pass snowman_word_dic to the get_letter_from_user function, but when you find a letter in the word, you update the correct_letter_guess_statuses. This actually causes an issue where the get_letter_from_user function doesn't register when a correct letter is guessed a second time. During play testing, this means that I'm never notified if I am picking a letter that has already been discovered. While this will pass all the tests, it does mean the program doesn't quite behave as expected.

[gina0song]

Got it! I plan to try pass only correct_letter_guess_statuses to get_letter_from_user(), like this : valid_character = get_letter_from_user(correct_letter_guess_statuses, wrong_guesses_list)

[apradoada]

One thing I would recommend here is to include some sort of print statement that lets the user know whether or not the letter they have guessed is in the word or not! Also, as mentioned before, if we modify the while loop conditional to use the len function, we can remove wrong_guesses_count altogether.

[gina0song]

Thanks for the suggestion! I will add something like : print(f"Good guess! '{valid_character}' is in the word."). and print(f"Sorry, '{valid_character}' is not in the word.")

[apradoada]

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:

1. Place both in a compound conditional within the while loop.
2. Use one as the while loop conditional and nest the other conditional within the while loop.

You have opted for the latter, which I think makes sense for a few reasons:

1. 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.
2. If we meet one of the conditions inside the loop, we can exit the function early (which you did)! This is important because you've specifically chosen to guide your loop with the condition that has a higher threshold to meet.

[apradoada]

Great idea to use a return statement here! This will stop the function early as soon as we find the correct word. This is a great approach to use where possible!

One small tweak to make lies in the difference between an implicit and explicit return statement. If we don't include the return keyword within our function, it will return None by default. This is what would be known as an implicit return. If we include the keyword as you have done here to prematurely stop the function, we have an explicit return.

Both are valid ways to exit a function, but best practices ask us to remain consistent in which one we use within a function. While you have an explicit return here, there is technically an implicit return after line 46. The explicit return here is necessary given how you’ve organized your logic, so it would be best practice to include a return statement at the end of the function as well.

[gina0song]

Got it, I will add a return at the end of the function as well.

[apradoada]

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 (correct_letter_guess_statuses).

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!

[gina0song]

Since dictionary lookups are faster than scanning a string, and using an extra dictionary would be unnecessary, I decided to change if valid_character in snowman_word_dic: to if valid_letter in correct_letter_guess_statuses:.

[apradoada]

Feel free to remove pass statements completely!