Sphinx - Sarah Koch #25
Conversation
…w_letters() in game.py was refactored to avoid using shuffle() based on instructor feedback.
| @@ -64,7 +64,7 @@ def test_letter_not_selected_too_many_times(): | |||
|
|
|||
| # Assert | |||
| for letter in letters: | |||
| assert letter_freq[letter] <= LETTER_POOL[letter] | |||
| assert letter_freq[letter] <= LETTER_POOL[letter] | |||
There was a problem hiding this comment.
Was this added white space an intentional change? It doesn't seem like it's needed, but just checking.
As a matter of keeping your PRs tidy, at internship, it's good practice PRs as concise as possible by only pushing up changes related to the feature changes being made.
Before I run git add/git commit/git push, I run git status and look at the files that were changed to make sure I didn't change accidentally change files besides the ones I intentionally wanted to change. Sometimes I'll also run git diff and do a quick skim to make sure my changes look as expected (harder to do with huge changes but that's another reason to make small frequent commits).
Anyways, this is all to say, if the white space on line 67 wasn't done on purpose then it's not a big deal to me that it was pushed up. However, if this was production code I'd tell you to remove it because we don't want to bring in accidental changes and checking in additional file makes you PR cluttered.
| list: A list of letters populated at their defined frequencies. | ||
| ''' | ||
| LETTER_POOL = { |
There was a problem hiding this comment.
👍 Nice job naming this as a constant variable.
| } | ||
|
|
||
| return [letter for letter, freq in LETTER_POOL.items() for _ in range(freq)] |
There was a problem hiding this comment.
I like the list comprehension here, it's Pythonic and concise! If someone asked you what this line of code was doing, how would you explain it?
| @@ -1,11 +1,168 @@ | |||
| import random | |||
|
|
|||
| NUM_DRAWN = 10 | |||
There was a problem hiding this comment.
👍 I like to see this value referenced by a descriptively named variable
| for letter in letter_bank: | ||
| copied_list.append(letter) |
There was a problem hiding this comment.
How would you use list comprehension to append the letters to copied_list?
You might also see the copy module used for making a new list by copying all the elements from an original list. Check out copy.deepcopy(list)
| BONUS_LENGTH = 7 | ||
| BONUS_POINTS = 8 |
There was a problem hiding this comment.
👍 Again, good work using constants for these values to make your code self-documenting.
| BONUS_LENGTH = 7 | ||
| BONUS_POINTS = 8 | ||
|
|
||
| total_score = sum(POINT_VALUES.get(letter.upper(), 0) for letter in word) |
There was a problem hiding this comment.
Nice and concise! What's going on in this line of code?
What's the data type of the object you pass to the sum function?
While the syntax on line 79 looks like list comprehension, it isn't.
It is interesting to think about the kind of iterable we should pass to a function like sum. For such a small data set like we have here it doesn't really matter, but have a look at this SO thread to read more about when we'd want to pass a list to sum instead.
| ''' | ||
| Checks to see if the word has the maximum possible length. | ||
|
|
||
| Parameters: | ||
| word (str): The word to check. | ||
|
|
||
| Returns: | ||
| bool: True if the word is of max length, otherwise False. | ||
| ''' | ||
| return len(word) == NUM_DRAWN |
There was a problem hiding this comment.
Since this function contains a small amount of code len(word) == NUM_DRAWN I think it's ok to not have it in the helper function and just directly it on lines 151 and 153:
if len(current_winner) == NUM_DRAWN:
return current_winner
if len(word) == NUM_DRAWN or len(word) < len(current_winner):
return wordHaving the helper does make tie_breaker a little more readable so maybe it's worth keeping in.
These kinds of comments will come up in a review. If I had a code review and got this comment, I'd either make the change to get rid of the helper function or I'd reach out to the reviewer and explain why I want to keep the helper function.
| return current_winner |
There was a problem hiding this comment.
Do we need lines 151 and 152 if you go on to return current_winner anyways on line 156?
| break |
There was a problem hiding this comment.
How would this logic work if you had a hand where there's a tie among ten letter words (maybe something like words = ["AAAAAAAAAA", "BBBBBBBBBB", "XXXXXXXXXX"]. Would this loop pick "XXXXXXXXXX" as the winning word?
How could you refactor your code to account for this scenario?
| if len(word) >= BONUS_LENGTH: | ||
| total_score += BONUS_POINTS |
There was a problem hiding this comment.
While a hand can't be more than 10 letters and therefore a word can't be more than 10 letters, you could make this check more explicit and conform to the instructions in the README more by having the check on line 81 be: if BONUS_LENGTH <= len(word) <= 10:
And put that 10 in a constant too.
No description provided.