Sapphire - Ericka M #118
Sapphire - Ericka M #118
Conversation
tildeee
left a comment
tildeee
left a comment
There was a problem hiding this comment.
Nice work on this, Ericka. Because wave 4 wasn't quite finished, I'm marking this as yellow as a sign to review.
Overall, your approaches to all of the completed functions are exactly what I'm looking for. Your code is overall clean and straightforward. I've added a few comments for ways I think your code style can improve. If it begins with [nit], it's because it's feedback that is a small code style issue that I'm nitpicking.
I've given some feedback on where I think get_highest_word_score could go, so let me know if you have questions!
Additionally:
- Keep using
printstatements to test things in your code! :) However, next time there's a final project submission, it will be good to catch those and delete them. - For your git commits, I would expect that the messages start with a verb and describe the changes in the code, and not reference "Wave 2" etc.
That being said, good work on this project. This is my first time getting to know your code, and I know I'm a new reviewer. Please let me know what questions or comments you have, here or through Slack.
| letter_score = (letter_pool_score[letter]) | ||
| letter_scores.append(letter_score) |
There was a problem hiding this comment.
letter_score is only being used once here, so I think it feels appropriate to inline this. For example, we could refactor this to:
letter_scores.append(letter_pool_score[letter])| letter_scores = [] | ||
| word = word.upper() | ||
| for letter in word: | ||
| letter_score = (letter_pool_score[letter]) |
There was a problem hiding this comment.
[nit] as a minor note, I'm unsure why there are () here. For me, I think this code reads better without the ().
| for letter in word: | ||
| letter_score = (letter_pool_score[letter]) | ||
| letter_scores.append(letter_score) | ||
| if (len(word)) >= 7: |
There was a problem hiding this comment.
[nit] Similar to above, I would expect this to be if len(word) >= 7:. When we change it, the tests still pass!
| letter_scores = [] | ||
| word = word.upper() | ||
| for letter in word: | ||
| letter_score = (letter_pool_score[letter]) | ||
| letter_scores.append(letter_score) | ||
| if (len(word)) >= 7: | ||
| letter_scores.append(8) | ||
| return sum(letter_scores) |
There was a problem hiding this comment.
Here, you've used letter_scores to hold all of the scores, and then you sum them up in the end. Instead of letter_scores, could we use an integer, and add to that integer instead?
| word_score = score_word(word) | ||
| winning_word_and_score = (word, word_score) | ||
| word_scores.append(winning_word_and_score) | ||
| highest_score = max(word_scores) |
There was a problem hiding this comment.
I see the makings of a working get_highest_word_score function!!! To keep pushing on this, I would ask the following questions: (PS: these are all style questions, there is no right answer)
- It looks like you're trying to make
word_scoresa list of tuples, where each item is a(word, score). From a list of tuples, how can you look atscoreinside that tuple? - There is only one highest
winning_word_and_score. When should thatwinning_word_and_scorebe re-assigned?
|
|
||
| def uses_available_letters(word, letter_bank): | ||
| pass | ||
| letters_copy = letter_bank[:] |
There was a problem hiding this comment.
Cool way to use slicing to make a copy
| import random | ||
| import copy | ||
|
|
||
| letter_pool_frequency = { |
There was a problem hiding this comment.
Because these variables don't get re-assigned, it might be good to make them constants
2 participants
No description provided.