Ada C22 Sphinx Class- Salma Anany #41
Conversation
One test in wave one failing due to randomness not being enough
| .idea | ||
| *.iml |
There was a problem hiding this comment.
When files are already being tracked by git, even after you add them to .gitignore they will continue to be tracked.
Be sure to review what files you have staged for commit before you add them so that you don't add files that shouldn't be tracked to the commit history.
There was a problem hiding this comment.
Check out this resource to review how to untrack files that you do not want added to a commit/pull request.
| @@ -1,11 +1,99 @@ | |||
| import random | |||
|
|
|||
| letter_counts = { | |||
There was a problem hiding this comment.
Since the values in this variable do not change, we should name the variable with all capital letters to indicate to other developers that this is a constant. So letter_counts should be LETTER_COUNTS.
Read more about constant variables here.
| def draw_letters(): | ||
| pass | ||
| all_letters = [] | ||
| hand= [] |
There was a problem hiding this comment.
Missing whitespace before the equal sign. It should look like hand = []
It's a small thing, but as you write more code (and eventually contribute to a large codebase) it's important to be consistent with whitespaces so that the whole project stays neat. Here's the official Python style guide that covers whitespaces.
| pass | ||
| all_letters = [] | ||
| hand= [] | ||
| for key in letter_counts: |
There was a problem hiding this comment.
Prefer a more descriptive name than key for this variable. What does the key of the letter_counts dictionary represent?
How about naming it letter so it's like
for letter in letter_counts:
# rest of your code| pass | ||
|
|
||
| score_list= [] | ||
| for char in word.upper(): |
There was a problem hiding this comment.
👍 nice job calling upper on word in the for-loop
| if char in score_chart.keys(): | ||
| score_list.append(score_chart[char]) | ||
| total_score = sum(score_list) | ||
| if len(word) >= 7: | ||
| total_score = total_score + 8 | ||
|
|
||
| return total_score |
There was a problem hiding this comment.
Do we need score_list to sum the scores to find total_score? We can remove this extra data structure and reduce complexity by directly getting the score.
Something like:
total_score = 0
for letter in word.upper():
total_score += SCORE_CHART[letter]
if len(word) >= 7:
total_score += 8
return total_score| if len(word) >= 7: | ||
| total_score = total_score + 8 |
There was a problem hiding this comment.
Prefer the literal integers 7 and 8 to be referenced by variables to make your code more self-documenting:
BONUS_LENGTH = 7
BONUS_POINTS = 8
if len(word) >= BONUS_LENGTH:
total_score += BONUS_POINTSThere 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 80 be: if 7 <= len(word) <= 10:
| winning_score = score | ||
| winning_word = word | ||
| return winning_word, winning_score |
There was a problem hiding this comment.
👍 Nice job directly returning a tuple on line 99.
| winning_score = score | ||
| winning_word = word |
There was a problem hiding this comment.
I think I'd prefer lines 96-98 to come before lines 93-95 because the second check is assigning some value to winning_score and winning_word after they are initialized.
if winning_score < score:
winning_score = score
winning_word = word
elif winning_score == score and len(winning_word) != 10:
if len(word) == 10 or len(winning_word) > len(word):
winning_word = word
No description provided.