Sapphire- Janice N. #115
Sapphire- Janice N. #115
Conversation
There was a problem hiding this comment.
Great work on your project, Janice!
Overall, your code is clear, and I can see really good manipulation of lists/dictionaries, and good and efficient loops. From my point of view, the biggest thing is working on the tie-breaking logic of wave 4. The second biggest thing is checking through this bug that I found.
I've added a few comments about small, optional style suggestions. I've marked these as "[nit]", or "nitpicky."
One last comment: Your git commit frequency is good, so keep that up. However, I would expect commit messages to start with a verb, describe the changes in the code within the commit, and to not describe "Wave 2 passing" etc. As your coworker dev, I want to be able to know what kind of code I can see if I look into the commit.
That being said, good work on this project, because overall the code looks good! I'm marking this project as "yellow" as a sign to review it. This is my first time getting to know you and your code, and I know I'm a new reviewer. Please let me know what questions or comments you have, here or through Slack.
| list_letter_pool =[] | ||
| for letter, number in LETTER_POOL.items(): | ||
| letter_quantity = letter * number | ||
| list_letter_pool.append(letter_quantity) | ||
|
|
||
|
|
||
| letter_string = ''.join(list_letter_pool) | ||
|
|
||
| # Available letters with frequencies list | ||
| available_letters = list(letter_string) |
There was a problem hiding this comment.
In this function, it looks like you're building a list of lists in list_letter_pool. Later on, you flatten list_letter_pool into a single list of strings in available_letters.
I think it might be worth thinking about how to refactor this logic-- while we iterate through LETTER_POOL.items(), is there a way we can create available_letters (without list_letter_pool)?
|
|
||
| hand = [] | ||
|
|
||
| # Returns a list of 10 random letters |
There was a problem hiding this comment.
[nit] This comment should be indented correctly
| list_letter_pool.append(letter_quantity) | ||
|
|
||
|
|
||
| letter_string = ''.join(list_letter_pool) |
There was a problem hiding this comment.
Nice use of join to flatten/combine the string!
| while len(hand) != 10: | ||
| random_letter = random.choice(available_letters) | ||
| hand.append(random_letter) | ||
| for letter in available_letters: | ||
| if letter in hand: | ||
| available_letters.remove(letter) |
There was a problem hiding this comment.
Your logic ends up being a little bit buggy! I noticed this because when I ran your tests (I run them like 50 times as I grade lol), sometimes 5 tests failed, sometimes 6 tests failed for me.
I spent some time trying to learn about the bug and this is what I found. While len(hand) != 10, you want to:
- pick a random letter (random_letter)
- check if that random letter is available (aka in available_letters)
- Only if that random letter is available, then do we add it to the hand and remove it from available_letters
Your code currently does something else. While len(hand) != 10, it:
- pick a random letter (random_letter)
- always adds the random_letter to the hand
- Loops through all available_letters
- If the available letter is in the hand, then it remove it from available_letters
The key difference is when you add to hand and when you remove from available_letters
This is a fun bug to fix and an interesting refactoring of your code! Let me know what questions come up.
| for letter in word: | ||
| if letter in hand and word.count(letter) < hand.count(letter): | ||
| continue | ||
| elif letter not in hand or word.count(letter) > hand.count(letter): | ||
| return False |
There was a problem hiding this comment.
Good use of continue. In this case, I think our code still works if we get rid of the first if and only check the elif case (as an if case). For example, this still passes the uses_available_letters tests:
for letter in word:
if letter not in hand or word.count(letter) > hand.count(letter):
return False| 'X':8, | ||
| 'Q':10, | ||
| 'Z':10 | ||
| } # Gets the score of the word |
There was a problem hiding this comment.
[nit] This ending brace should be indented, and it might make sense to put the comment on its own line.
| score += points_value[letter] | ||
| if len(word) == 0: | ||
| score = 0 | ||
| if len(word) in bonus_point_list: |
There was a problem hiding this comment.
I actually really like how you made and used bonus_point_list. To be honest, if len(word) >= 7 would also pass the tests, which is a little more straightforward/direct... But if we ever updated this project, and needed to update the bonus point lengths, we would only need to update the one bonus_point_list variable.
| if len(word) == 0: | ||
| score = 0 | ||
| if len(word) in bonus_point_list: | ||
| score = score + 8 |
There was a problem hiding this comment.
Small suggestion: score += 8 works here too
| score = 0 | ||
| if len(word) in bonus_point_list: | ||
| score = score + 8 | ||
| return(score) |
There was a problem hiding this comment.
[nit] I think it would be better and more consistent code style to return score without parens and with a space.
| if value == highest_score: | ||
| score_list.append(word) | ||
| score_list.append(highest_score) | ||
|
|
||
| return tuple(score_list) |
There was a problem hiding this comment.
Nice work! It seems like the details about the tie-breaking is what's next in this project. From what I can tell, I think that your next step here could be:
- You've found the
highest_scoreand all of theword,values that match thehighest_score. Instead ofappending them toscore_list, maybe there's another way of storing them. - You want to store the tie-breakers because you eventually want to look at their word lengths. Just like how you found
highest_score, you can find the smallest word and also all words that have a length of 10.
No description provided.