Tigers, SAMIYA AND TANIL #72
Conversation
| letters = [] | ||
| remaining_letters_pool = {} | ||
|
|
||
| if len(letters) < 11: |
There was a problem hiding this comment.
This is a redundant if statement. Given the fact that you've initialized letters to be an empty list on line 31, the length will always be less than 11! Therefore we can remove this if statement.
| for dict in LETTER_POOL: | ||
| for key, value in dict.items(): | ||
| if key not in remaining_letters_pool and value > 0: | ||
| remaining_letters_pool[key] = value |
There was a problem hiding this comment.
If I'm reading this correctly, you are copying over all the values from LETTER_POOL into remainig_letters_pool. We have ways to do that which have already been optimized for us. You could have simply used the deepcopy() function to copy the dictionary over.
| while len(letters) < 10: | ||
| random_letter = random.choice(list(remaining_letters_pool)) | ||
| ##Removing the used letter from the remaining letter pool | ||
| for letter in list(remaining_letters_pool): |
There was a problem hiding this comment.
If you do the suggestion from the last comment, you know for certain that the random_letter you get will be a key in the remaining letters pool. Therefor we don't have to loop through the list of keys again here. We can just access each key normally, comfortable in the fact that we are working with a key that exists.
| remaining_letters_pool.pop(letter) | ||
| elif random_letter == letter: | ||
| remaining_letters_pool[letter]-= 1 | ||
| letters.append(random_letter) |
There was a problem hiding this comment.
Following the advice from above, we can simplify this into a single if statement by simply checking if the random_letter has values left in the dictionary:
if remaining_letters_pool[random_letter] > 0:
remaining_letters_pool[random_letter] -= 1
letters.append(random_letter)
| return False | ||
| else: | ||
| return True | ||
|
|
There was a problem hiding this comment.
I see what y'all were trying to do here, and the code works and shows a good understanding of lists and dictionaries, but there's a simpler way to approach this problem and it starts with creating a deep copy of the letter bank. That would allow us to have a copy of the letter bank that we can modify without modifying the original. From there, you could simply loop through each letter in the word and check to see if it is in the copied letter_bank. If it is in there, remove the letter from the letter bank and move to the next one (That'll take care of duplicate letters). If it isn't there, we can immediately return false. That would look something like this:
for letter in word:
if letter in letter_bank_copy:
letter_bank_copy.remove(letter)
else:
return False
return True
| total_sum=8 | ||
| for letter in cap_word: | ||
| total_sum+=letter_value[letter] | ||
| return total_sum |
There was a problem hiding this comment.
This looks great! My only suggestion would be to make the letter_value dictionary a constant outside of these functions.
| else: | ||
|
|
||
| return (words[0], highest_score) |
There was a problem hiding this comment.
I once again get where y'all are coming from with this code, and it does look like it works, but there are a few strategies you could use to make this code more efficient/easier to read. The biggest tip I have with something like this is to recognize that you can do all the comparisons as you move through the list. I actually really like what y'all did to find the highest score and any words that reach that highest score, but it is somewhat unnecessary. A more efficient algorithm would start by creating a tuple to hold the highest scoring word and its value. Then you go through each word, score it, and then compare the score of that word with what's in the tuple using the for loops to figure out which one is correct. Doing the comparisons as you go along will make everything just that much simpler.
No description provided.