Tigers - Mica C. and Lain #64
Conversation
Co-authored-by: allcomputersarebad <[email protected]>
|
note tests fail due to use of counts= named parameter of random.sample, which was added in python version 3.9 |
There was a problem hiding this comment.
Good work!
I really appreciate y'all's succinctly written code while still being very readable, as well as a correct implementation of the requirements. I do make a few comments about what I think are performance issues, but in one case there's an argument in favor of y'all's implementation, and in the other a simple fix. I think I also make some positive comments with slight quibbles about readability.
| "Z": 10, | ||
| } | ||
|
|
||
|
|
||
| def draw_letters(): |
There was a problem hiding this comment.
This looks fine, good use of library functions and built-in operators! No comments.
|
|
||
| pool = "".join(letter * count for letter, count in LETTER_POOL.items()) | ||
| return sample(pool, k=10) | ||
|
|
||
|
|
||
| def uses_available_letters(word, letter_bank): |
There was a problem hiding this comment.
This is a fine implementation very succinct and pythonic, though there's a slight time performance hit from using count...
| upper_word = word.upper() | ||
| return all( | ||
| upper_word.count(letter) <= letter_bank.count(letter) | ||
| for letter in set(upper_word) |
There was a problem hiding this comment.
From my investigations, count takes O(n) time as the implementation goes through each element of the list and just counts up. The for generator runs over set(upper_word), which at worst has n elements (n being the number of letters in word) if every letter in word is different. Thus O(n^2) in the end for time complexity.
A common way to implement this function is to create frequency dictionaries of either or both word and letter_bank (each likely taking a separate O(n) for loop) and then compare the counts, which could also be done in an O(n) loop. Since none of these loops are nested, the entire implementation takes O(n).
However, by creating the dictionaries, we are also using O(n) space, while the big benefit of generators is that they don't need to allocate space, making y'all's implementation O(1). Which could very well be worth the worse time complexity! Trade-offs abound.
| upper_word.count(letter) <= letter_bank.count(letter) | ||
| for letter in set(upper_word) | ||
| ) | ||
|
|
||
|
|
||
| def score_word(word): |
There was a problem hiding this comment.
This is also a very fine succinct and pythonic implementation, but I think then we get a space performance issue this time...
|
|
||
| def score_word(word): | ||
| pass | ||
| total_score = sum([LETTER_POINTS.get(letter, 0) for letter in word.upper()]) |
There was a problem hiding this comment.
In this case rather than a generator, y'all used a comprehension (due to the square brackets), which does end up allocating O(n) memory (apparently even if you never assign it, sadly). However, I just tested and y'all could just switch to a generator here and get O(1) space complexity!
Great since this is so succinct and clean and I do really value that!
|
|
||
| def score_word(word): | ||
| pass | ||
| total_score = sum([LETTER_POINTS.get(letter, 0) for letter in word.upper()]) | ||
| total_score += len(word) > 6 and 8 |
There was a problem hiding this comment.
Good use of the short-circuiting behavior of and as well as the integer representation of False as 0. I could quibble about readability, but these behaviors are common enough knowledge it's not that big a deal to me.
| return total_score | ||
|
|
||
|
|
||
| def break_tie(words): |
|
|
||
|
|
||
| def break_tie(words): | ||
| return min(words, key=lambda w: 0 if len(w) == 10 else len(w)) |
There was a problem hiding this comment.
And this is so good and succinct to account for all tiebreakers and verifying that min (and its siblings) pick the first encountered matching value it does indeed meet all the tie breaking rules.
Nice work!
|
|
||
| def break_tie(words): | ||
| return min(words, key=lambda w: 0 if len(w) == 10 else len(w)) | ||
|
|
||
|
|
||
| def get_highest_word_score(word_list): |
There was a problem hiding this comment.
Nice and succinct as all the rest of the code has been while still being correct. No comments.
https://www.youtube.com/watch?v=3wjDkFA-FQo