Sunitha N -Adagrams-ADA-C22-Sphinx#46
Conversation
nelasunitha
changed the title
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,150 @@ | |||
| import random | |||
There was a problem hiding this comment.
Usually we leave a blank line after imports
You can review the official Python style guide about blank line conventions.
| import random | ||
| 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
|
|
||
| total_letters = sum(letter_pool.values()) | ||
|
|
||
| while len(drawn_letters) < 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 line 49 and wonder what the significance of 10 means.
How about something like:
HAND_SIZE = 10
while len(drawn_letters) < HAND_SIZE:|
|
||
| while len(drawn_letters) < 10: | ||
| # Generating a random number between 1 and the total number of letters | ||
| rand_num = random.randint(1, total_letters) |
There was a problem hiding this comment.
While randint() does return a random number, rand_num doesn't quite describe what this random integer represents in our game. A name like rand_index is a little more descriptive.
| letter_pool[letter] -= 1 | ||
| total_letters -= 1 | ||
| break | ||
|
|
There was a problem hiding this comment.
I think one blank line after break is ok to help separate the logical parts of this function, but having 2 is not needed. Line 62 can be removed.
| def get_highest_word_score(word_list): | ||
| pass No newline at end of file | ||
| max_word = word_list[0] | ||
| max_word_score =score_word(max_word) |
There was a problem hiding this comment.
Missing white space after equal sign
| max_word = word_list[0] | ||
| max_word_score =score_word(max_word) | ||
| #iterating from the second word | ||
| for word in word_list[1:]: |
There was a problem hiding this comment.
👍 nice job slicing off the first word since it's already defined as the max_word on line 132.
| max_word = word | ||
|
|
||
| max_word_score = max(max_word_score, word_score) | ||
| return (max_word,max_word_score ) |
There was a problem hiding this comment.
Missing white space after comma and there's an extra white space that needs to be removed after max_word_score.
| # words with same score | ||
| elif word_score == max_word_score: | ||
| # Prefer the word with the fewest letters, unless one has 10 letters | ||
| if len(word) == 10 and len(max_word) != 10: |
There was a problem hiding this comment.
We could use a constant variable to reference 10, maybe something like TIE_BREAKER_LENGTH = 10 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 | ||
|
|
||
| max_word_score = max(max_word_score, word_score) |
There was a problem hiding this comment.
Do we need line 149 when you reassign the value of max_word_score to be the current highest score on lines 124 - 126?
2 participants
Creating pull request of Adagrams to review for Sunitha Nela