Snow Leopards - Jessica Hebert & Jennifer Nouel #81
Conversation
Co-authored-by: Jessica Hebert <[email protected]>
There was a problem hiding this comment.
Nice work on your first pair project, Jennifer and Jessica!
All your tests are passing and your code is laid out well overall.
Your variables are clearly named and the code is concise which makes your project readable.
I called out a couple of pieces of your solution for you two to check out because the current implementation does affect the time complexity. Let me know if you have any questions!
| @@ -1,11 +1,144 @@ | |||
| from multiprocessing import pool | |||
There was a problem hiding this comment.
Looks like this import is unused so you can delete it. Perhaps it was accidentally brought in by VSCode. There should be an option to turn off auto completion for imports to avoid bringing in stray, unused imports
| from multiprocessing import pool | ||
| import random | ||
|
|
||
| def build_letter_pool(): |
There was a problem hiding this comment.
I see that this helper function returns the letter pool which keeps the other function bodies concise. I think when you have a large data structure like this, it is helpful to pull them out of the functions that use them so they don't get dominated by the data structure.
However, since LETTER_POOL is a constant variable, you can have it as a global variable in game.py without wrapping it in a helper function.
| available_letters = [] | ||
|
|
||
| for letter,frequency in LETTER_POOL.items(): | ||
| for i in range(frequency): | ||
| available_letters.append(letter) | ||
|
|
||
| return available_letters |
There was a problem hiding this comment.
Nice helper function to keep draw_letters a single responsibility function and more concise 👍
|
|
||
| def uses_available_letters(word, letter_bank): | ||
| pass | ||
| letter_bank_copy = letter_bank.copy() |
There was a problem hiding this comment.
Another Pythonic way you'll see to make a shallow copy is:
letter_bank_copy = letter_bank[:]
| for letter in word.upper(): | ||
| if letter not in letter_bank_copy: |
There was a problem hiding this comment.
Let's have a closer look at the time complexity involved with lines 68-69.
Line 68: a for loop that loops as many times as there are letters in word - O(n)
Line 69: in operator that in the worst case scenario has to check every letter in letter_bank_copy to see if letter is in it - O(n)
Rather than using a list of the letters in the hand, could we build a helper data structure (like a dictionary) that could let us look up if a letter was part of a user's hand?
If the helper dictionary had keys that were the letters and the values were the number of letters in the hand, then you could leverage the fact that key look up in dictionaries is a constant O(1) operation.
So on line 69, you can check if key (the letter in the hand) in helper_dictionary, which has constant look up time.
Additionally, instead of using the remove method, you could decrement the value in the dictionary to keep track of how many letters have been played. Recall that the remove method on a list also has linear run time because it could have to iterate over the entire list before finally removing letter.
| |Q, Z | 10 | | ||
| """ | ||
|
|
||
| score_chart = { |
There was a problem hiding this comment.
Since this helper just returns a constant variable, you can make score_chart a global constant called SCORE_CHART that is declared at the top of this file.
|
|
||
| for letters, points in score_chart.items(): | ||
| for char in word.upper(): | ||
| if char in letters: |
There was a problem hiding this comment.
Since letters is a tuple, when you use the in operator you'll encounter linear time complexity.
To avoid increasing time complexity, you could refactor score_chart so that it is a dictionary where the keys are letters and the values are integers (neither keys or values would be list or tuple which both have linear time complexity for in).
If you refactor score_chart as suggested, then you can leverage a dictionary's constant time look up to see if a key (letter) is in the dict. That means your dictionary would be longer, but if you declare it as a constant global variable at the top of the file, it won't clutter your method here and it's worth having a longer dict, but quicker look up time.
| @@ -73,6 +79,7 @@ def test_get_highest_word_tie_prefers_ten_letters_unsorted_list(): | |||
| assert best_word[0] == "AAAAAAAAAA" | |||
| assert best_word[1] == 18 | |||
|
|
|||
| #@pytest.mark.skip() | |||
There was a problem hiding this comment.
Since the changes in your test files are just to comment out the skip test decorate, you don't technically need to add and commit these changes (but it doesn't hurt anything to do so). You could just add and commit the changes in game.py and that would make your pull request more concise with just the logical changes you made.
No description provided.