Panthers - Jamaica A. #73
Conversation
|
Deleted play_test.py in adagrams folder to match Ada-C18:master and prevent errors when running test for url link in Learn/galvanize |
|
|
||
|
|
||
|
|
||
| score_chart = { |
There was a problem hiding this comment.
Placed out here as a global constant, I would recommend using all caps naming: SCORE_CHART.
| @@ -1,11 +1,134 @@ | |||
| import random | |||
|
|
|||
| LETTER_POOL = { | |||
There was a problem hiding this comment.
Since LETTER_POOL and score_chart are each accessed by one function, it would be reasonable to place them inside the functions rather than as a constant. There are tradeoffs, the structures would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.
| while len(new_list) <10: | ||
| letter = (random.choices(list(LETTER_POOL.keys()), weights = list(LETTER_POOL.values()), k=1))[0] |
There was a problem hiding this comment.
Nice use of random.choices!
This line gets pretty long, to keep it under the PEP8 recommendation of 79 characters we should split it up over a couple lines. Something we could do that would also lighten the processing we do each iteration could be pulling the list creations out of this line and storing them in variables above the loop.
letters_list = list(LETTER_POOL.keys())
letter_weights = list(LETTER_POOL.values())
while len(new_list) <10:
letter = random.choices(letters_list, weights = letter_weights, k = 1)[0]| if new_list.count(letter) < LETTER_POOL[letter]: | ||
| new_list.append(letter) | ||
|
|
||
| return(new_list) |
There was a problem hiding this comment.
We want to be consistent in how we do things across a project and remove syntax we don't need. I'd recommend always placing a space between the return keyword and the content that follows, and removing surrounding parentheses unless they are required to separate multiple expressions.
| letter_bank_copy = letter_bank.copy() | ||
| if len(word) > len(letter_bank_copy): | ||
| return False |
There was a problem hiding this comment.
Nice quick check to rule out words with an invalid length!
This implementation will allocate space for letter_bank_copy before we know if the input word is too long to be valid. If line 80 checked against the length of the input letter_bank, then we could move the if statement to the top of the function, and only create the deep copy if we know we need it.
if len(word) > len(letter_bank):
return False
letter_bank_copy = letter_bank.copy()|
|
||
| return(winner, winner_score) |
There was a problem hiding this comment.
The variable top_score is already holding the max score for our tied words. This means we could remove line 131 and use top_score on line 133:
return(winner, top_score)|
|
||
| if len(winner_list) == 1: | ||
| winner = (winner_list[0]) |
There was a problem hiding this comment.
Great thought to check the length of winner_list to see if we can exit early!
| winner2 = min(winner_list, key=len) |
There was a problem hiding this comment.
winner1 and winner2 as names don't tell us much about the difference between these variables. I would consider using names that give the reader more information. One of many possibilities could be longest_winner & shortest_winner.
| winner = winner1 | ||
| else: | ||
| winner = winner2 |
There was a problem hiding this comment.
Really nice use of min and max to tiebreak on length!
| winner = (winner_list[0]) | ||
|
|
||
| if len(winner_list) > 1: |
There was a problem hiding this comment.
Since we know we will never need to enter the if here on line 123 if we entered if len(winner_list) == 1: on line 120, I would consider using an elif here. With an elif, if we hit line 120 and enter if len(winner_list) == 1:, then the code knows we entered the if-statement and will not bother evaluating the following connected elif statements, saving us the tiniest bit of processing.
No description provided.