Sphinx - Vlada Rapaport#31
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great job on Adagrams!
In the future, consider making a commit after finishing each wave.
Let me know if you have questions about my comments!
| @@ -1,11 +1,145 @@ | |||
|
|
|||
There was a problem hiding this comment.
Delete unneeded blank line
You can review the official Python style guide about blank line conventions.
|
|
||
| def draw_letters(): | ||
| pass | ||
| letter_pool = { |
There was a problem hiding this comment.
letter_pool is a constant variable and should be named with capital letters like LETTER_POOL
| letters = "" | ||
| for letter , num in letter_pool.items(): | ||
| letters += letter * num |
There was a problem hiding this comment.
Your implementation of draw_letters has 2 responsibilities: creating a pool of letters and also drawing random letters. Consider putting the logic on lines 33-35 into a helper function (maybe something like build_letter_pool and call that function here instead.
| for letter , num in letter_pool.items(): | ||
| letters += letter * num | ||
|
|
||
| result = [] |
There was a problem hiding this comment.
The name result doesn't quite capture what this list represent in the game. Something like randomly_hand describes that this list is a random hand in the game which would make your code more self-documenting.
| } | ||
|
|
||
| while len(result) < 10: | ||
| i = random.randint(0, len(letters) - 1) |
There was a problem hiding this comment.
What does i represent? Something like random_index would be more descriptive.
| if len(word) >=7 and len(word) <= 10: | ||
| sum_word += 8 |
There was a problem hiding this comment.
Using constant variables will make your code more self-documenting and easier to understand. If I don't know anything about this game, I'm left wondering what the significance of the integer literals 7, 10, and 8 are.
Consider something like:
BONUS_LENGTH_MIN = 7
BONUS_LENGTH_MAX = 10
BONUS_POINTS = 8
if BONUS_LENGTH_MIN <= len(word) <= BONUS_LENGTH_MAX:
sum_word += BONUS_POINTS| 'Z': 0 | ||
| } | ||
|
|
||
| while len(result) < 10: |
There was a problem hiding this comment.
I'd like to see the literal integer 10 referenced by a constant variable. As someone who might not know anything about this game, I might read line67 and wonder what the significance of 10 means.
How about something like:
HAND_SIZE = 10
while len(result) < HAND_SIZE:|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Delete blank lines at end of file to keep project neat. Python recommends you keep one blank line so you can remove lines 143-145
You can check out this thread to read more if you'd like
| max_score = score | ||
| max_word = word | ||
| elif score == max_score and (len(word) != len(max_word)): | ||
| if len(word) == 10: |
There was a problem hiding this comment.
We could use a constant variable to reference 10, maybe something like TIE_BREAKER_LENGTH or something else to describe why 10 is significant in this logic.
| elif len(word) < len(max_word) and len(max_word) != 10: | ||
| max_word = word | ||
|
|
||
| return (max_word, max_score) |
There was a problem hiding this comment.
👍 Nice job directly returning the tuple
2 participants
No description provided.