Conversation
Co-authored-by: misha-joy <misha-joy@users.noreply.github.com>
spitsfire
left a comment
spitsfire
left a comment
There was a problem hiding this comment.
Great job working together, Reyna and Misha!
Here are a few things to consider for future projects that I've detailed below in your code:
- There were a few places in Wave 1 that the logic was incomplete. Unfortunately, the tests didn't quite catch it.
- Careful creating variables that aren't used. There were a few places where a variable was created and modified. But then that variable was never used to compare or calculate the final output.
If you have any questions about the feedback, please reach out to me!
| def uses_available_letters(word, letter_bank): | ||
| pass | ||
| for letter in letters_not_drawn: | ||
| if letter not in letters_drawn and len(letters_drawn) < 10: |
There was a problem hiding this comment.
hmm does this mean that the list of letters_drawn will never have duplicate letters? Plenty of words need to use the same letters. That's why the LETTER_POOL pull has values that represent how many of each letter can be pulled.
Instead of checking letter not in letters_drawn, we should check whether or not the letter has a quantity greater than 0 to still draw from.
|
|
||
| def uses_available_letters(word, letter_bank): | ||
| pass | ||
| for letter in letters_not_drawn: |
There was a problem hiding this comment.
I think we could combine the for loop and if statement in one using a while loop. Consider:
while len(letters_draw) < 10:
# randomly select a letter
# append to letters_drawn if that letter has any quantity left
# the rest of your codeBut what letter do we draw? Well, it's supposed to be random every time. Instead of running a for loop from start to finish, how can we randomly select a key or an index position? (This may take some Googling)
| for letter in word: | ||
| if letter in letter_bank and word.count(letter) <= letter_bank.count(letter): | ||
| word_dict[letter] = True | ||
| letter_count += 1 | ||
| is_valid = True | ||
| elif not letter in letter_bank: | ||
| word_dict[letter] = False | ||
| is_valid = False | ||
| return is_valid |
There was a problem hiding this comment.
I think we can get rid of the word_dict and letter_count altogether! Both are created and updated, but then they aren't used anywhere to calculate our final result.
Secondly, there's a logic error that the tests didn't pick up on. Let's take the word "ALABAMA" for example. Based on this function:
letter_bank = ["F","A","B","D","A","C","C","L","M","N"]
uses_available_letters("ALABAMA", letter_bank)it returns True, which it shouldn't. What's causing this? Well, let's take "A" through your conditionals.
if letter in letter_bank and word.count(letter) <= letter_bank.count(letter) 4<= 2 so, no this doesn't execute.
elif not letter in letter_bank "A" is in the letter_bank, so this doesn't execute either.
What can we add to this function to fix this?
| is_valid = False | ||
| return is_valid | ||
|
|
||
| def score_word(word): |
| longest_word = max(highest_scoring_words, key=len) | ||
| shortest_word = min(highest_scoring_words, key=len) |
There was a problem hiding this comment.
👍 nice job using max and min so effectively!
| score += 8 | ||
| return score | ||
|
|
||
| def get_highest_word_score(word_list): |
| for letter in letters_not_drawn: | ||
| if letter not in letters_drawn and len(letters_drawn) < 10: | ||
| letters_drawn.append(letter) | ||
| letter_count += 1 |
There was a problem hiding this comment.
Doesn't look like we are using this variable anywhere. Let's get rid of it!
| for letter in word: | ||
| if letter in letter_bank and word.count(letter) <= letter_bank.count(letter): |
There was a problem hiding this comment.
we are only just learning about Big O and a function's time and space complexity, but let's explore how we can make this more efficient.
Right now we have a loop inside a loop (if...in has a loop under the hood). That makes the code O(n^2). Consider calculating a frequency map of the letters before entering the for loop on line 23. Building a frequency map is a linear operation aka O(n), but we could look up the letters in constant time aka O(1) with a frequency map (if letter in dictionary versus if letter in list).
That's very technical, but to summarize: using a dictionary to look up each letter in the word and subtracting from the dictionary would be more time efficient than checking if the letter is in the letter_bank list.
3 participants
it's game time