Sphinx Class - Denise C.#36
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great job on Adagrams!
Let me know if you have questions about my comments!
| 'X': 8, | ||
| 'Y': 4, | ||
| 'Z': 10 | ||
| } |
There was a problem hiding this comment.
Nice job naming LETTER_POOL and LETTER_POINT_VALUES as constant variables 👍
|
|
||
| list_of_all_letters = [] | ||
|
|
||
| for letter,amount_of_that_letter in LETTER_POOL.items(): |
There was a problem hiding this comment.
Missing white space after the comma, should look like letter, amount_of_that_letter.
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.
There was a problem hiding this comment.
amount_of_that_letter is descriptive but might be considered a little lengthy. What about something like letter_freq?
| list_of_all_letters = [] | ||
|
|
||
| for letter,amount_of_that_letter in LETTER_POOL.items(): | ||
| for _ in range(amount_of_that_letter): |
There was a problem hiding this comment.
👍 good use of _ when we don't need an actual looping variable
|
|
||
| for letter,amount_of_that_letter in LETTER_POOL.items(): | ||
| for _ in range(amount_of_that_letter): | ||
| list_of_all_letters.append(letter) |
There was a problem hiding this comment.
How do you think you could use list comprehension to write the loop from lines 66-68 in one line?
| 'Z': 10 | ||
| } | ||
|
|
||
| def draw_letters(): |
There was a problem hiding this comment.
Notice that the draw_letters method has two responsibilities. It is creating a letter pool to draw from and then pulling random letters. To keep your methods to a single responsibility, consider pulling out the logic from lines 64-68 into a helper function and then invoking that helper function in draw_letters. Maybe something like create_letter_pool?
| for letter in word: | ||
| letter = letter.upper() |
There was a problem hiding this comment.
Same comment as above, call upper on word before looping
|
|
||
| def score_word(word): | ||
| pass | ||
| sum = 0 |
There was a problem hiding this comment.
While we are accumulating the score for each letter, a more descriptive name for this variable could be total_score
| def test_get_highest_word_many_ties_pick_first_shortest(): | ||
| # Arrange | ||
| words = ["LL", "NN", "RR", "AA"] | ||
|
|
||
| # Act | ||
| best_word = get_highest_word_score(words) | ||
|
|
||
| # Assert | ||
| assert best_word[0] == "LL" | ||
| assert best_word[1] == 2 | ||
|
|
||
| def test_get_highest_word_many_ties_pick_first_shortest_not_in_first_position(): | ||
| # Arrange | ||
| words = ["LL", "NN", "JQ", "AA"] | ||
|
|
||
| # Act | ||
| best_word = get_highest_word_score(words) | ||
|
|
||
| # Assert | ||
| assert best_word[0] == "JQ" | ||
| assert best_word[1] == 18 No newline at end of file |
|
|
||
| def get_highest_word_score(word_list): | ||
| pass No newline at end of file | ||
| winning_word_info = [] # ["word", score] index = [0, 1] |
There was a problem hiding this comment.
Remove debugging comments when you submit code. It's not a big deal here but when you go to industry, extraneous comments will clutter up huge codebases. Since your code is self-documenting with descriptively named variables and functions and is readable, we wouldn't need this comment to understand how the code works.
| winning_word_info_tuple = tuple(winning_word_info) | ||
|
|
||
| return winning_word_info_tuple |
There was a problem hiding this comment.
Instead of creating a list winning_word_info and then casting that list to a tuple, you can directly return a tuple like this:
return word, word_scoreWith this refactor you'd have to update the loop a little bit so you know the word_score and the word that corresponds to it.
Recall that tuple syntax can omit parens and just use commas to separate the elements of a tuple:
https://learn-2.galvanize.com/cohorts/4211/blocks/1032/content_files/representing-data/tuples.md
| for letter in word.upper(): | ||
| total_score += LETTER_POINT_VALUES[letter] | ||
| if len(word) >= 7: | ||
| total_score += 8 |
There was a problem hiding this comment.
Didn't call this out earlier, but your code would be more self-documenting by using constant variables here too:
BONUS_LENGTH_MIN = 7
BONUS_POINTS = 8
There 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 108 be: if BONUS_LENGTH_MIN <= len(word) <= BONUS_LENGTH_MAX:
Where BONUS_LENGTH_MAX is 10
2 participants
No description provided.