Spruce - Ngozi Amaefule #42
Conversation
…ing it to the returned draw_ten list
…he total value of the word
There was a problem hiding this comment.
Nice job!
All waves are complete and all tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. You used lists and dictionaries throughout, and were aware of when you needed to make copies to avoid side-effects.
The main thing I'd recommend thinking about, especially when working primarily with lists, is whether we can do a little pre-calculation here and there to build helper structures (often a frequency map of some kind stored in a dict) to improve the efficiency of our code. This is also true for data representation. Sometimes the most optimal way to store some data isn't the most optimal way to perform our needed lookups. And intermediate data transformation can give us the best of both worlds!
Also, watch out for untested code in your projects. We'll learn a bit about code coverage shortly, but even without using a tool to help us with coverage, we should try to make sure that any code we write has been run at least once (manually through some scratch code of our own, or by adding our own additional tests).
That being said, great job!
adagrams/game.py
Outdated
| def draw_letters(): | ||
| pass | ||
| pool_of_letters = { |
There was a problem hiding this comment.
Consider declaring this as a "constant" (POOL_OF_LETTERS) outside this function. Doing so wouldn't really save on runtime (since we need to modify the counts, we'd need to make a copy at the start of each call to the method), but it does declutter the method a bit by allow us to move the large data structure to some innocuous corner of our file.
adagrams/game.py
Outdated
|
|
||
|
|
||
|
|
||
| for i in range(10): |
There was a problem hiding this comment.
Rather than looping 10 times, we could loop until the draw_ten list gets 10 elements (while its length is less than 10). Doing so would allow us to avoid having to write the extra inner loop to find a letter that we still have a tile for. Instead, if the count for the "drawn" letter were 0, we could continue the loop to start again from the top and pick a new letter.
adagrams/game.py
Outdated
|
|
||
|
|
||
| for i in range(10): | ||
| random_letter = random.randint(0, 25) |
There was a problem hiding this comment.
Rather than hard coding a range of 25, consider calculating this based on the list we'll be using the result in. This would allow the code to automatically work correctly in the event that we changed the set of characters used for tiles without needing to hunt down and update literals (which might not be easy to find).
Here, we could replace 25 with len(list_of_letters) - 1
adagrams/game.py
Outdated
| elif len(word) == len(letter_list): | ||
| for letter in word: | ||
| if letter in letter_list: | ||
| letter_list.remove(letter) | ||
| if letter_list is False: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
During the review, I mentioned that I don't think there were any tests in the project that would exercise this condition (all of the words used in the uses_available_letters tests are shorter than the tile count). That being said, this condition could probably be cut entirely, since checking whether a word smaller than the length of tiles, and checking whether a word equal in length to the available tiles are nearly the same, and I'd expect the smaller word logic to work.
adagrams/game.py
Outdated
| for letter in word: | ||
| if letter not in letter_list: | ||
| return False | ||
| else: |
There was a problem hiding this comment.
Notice that if a previous branch in a conditional terminates with a return, then a subsequent elif could be written simply as if and an else can be omitted entirely. This is because if the code didn't return, we must have been in the "negative" branch, otherwise we wouldn't have reached this spot in the code (we would have returned!). For the else situation, that can save us a level of indentation and make the code a little easier to read.
adagrams/game.py
Outdated
| return False | ||
| else: | ||
| whole_word.append(letter) | ||
| letter_list.remove(letter) |
There was a problem hiding this comment.
remove is a linear operation because 1) it has to loop through the list to find the value to remove, and 2) it has to shuffle any "downstream" values forward to close any gap left after the removal. If instead we operated on a pre-calculated frequency map, you could track whether a letter is present/used up similar to how you enforced the tile pick counts in draw_letters, and each operation would be constant time!
adagrams/game.py
Outdated
| if len(whole_word) == len(word): | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Try to avoid writing code like:
if some_condition:
return True
else:
return FalseInstead, return the result of the condition directly, so here
return len(whole_word) == len(word)
adagrams/game.py
Outdated
| sum = 0 | ||
|
|
||
| if len(word) > 6: | ||
| sum += 8 |
There was a problem hiding this comment.
We could totally ternary this!
sum = 8 if len(word) > 6 else 0Also, we could also introduce some constants to avoid the bare literals, and we could try to avoid using sum as a variable name (since it's also the name of a global built-in python function). That could look like
LONG_WORD_BONUS = 8
LONG_WORD_BONUS_LENGTH = 7
BASIC_SCORE = 0
score = LONG_WORD_BONUS if len(word) >= LONG_WORD_BONUS_LENGTH else BASIC_SCOREIt may be a little longer, but it describes all the parts of the calculation.
adagrams/game.py
Outdated
| sum += 8 | ||
|
|
||
| for letter in word.upper(): | ||
| if letter in value_1: |
There was a problem hiding this comment.
Checking each letter in a range like this is definitely more maintainable than checking for each letter one by one, but consider using a dictionary to map each possible letter to a score. We could look up the score for each letter, and add it to our overall score!
Building the table is a little painful (and redundant), but we could imagine starting from a table that uses a string of all the letters which share a score a the key, and the score as the value. That would be very maintainable and have no redundancy. Then we could write code to convert from that structure to a dictionary where each letter is its own key. This would let us have a maintainable data representation, AND quick score lookups!
adagrams/game.py
Outdated
| shortest_word_length = 10 | ||
|
|
||
| for word, score in high_score_words.items(): |
There was a problem hiding this comment.
Interesting steps of finding all the scores for the words, finding the max score, finding all the words that have the max score. Then bailing out as soon as we find one with a length of 10, otherwise finding the length of the shortest word with the max score, then finding the first word with that shortest length. It's a little complex (some comments would help call out what you're doing along the way), but the complexity is still linear overall, placing this in the same complexity class as the most optimal possible solution. (We can't do any better than linear with respect to the number of words, since we need to inspect/compare the score of every word in the list!)
Notice that your use of dictionaries here works because dictionaries preserve their insertion order for iteration. We usually talk about dictionaries as being unordered, and it's tru that the key values have no ordered relationship, but since you built up the various dictionaries by iteration, we can depend on the order of the dictionary items to be the same as the order of that first word list!
No description provided.