Phoenix - Elzara T. #25
Conversation
There was a problem hiding this comment.
GREEN
Great job, Elzara! Your code looks great! I left some comments and some feedback, but really only small things! As far as revisions go, you are more than welcome to make changes if you would like, but that is not necessary! If you do make changes, though, just let me know and I'm happy to take a second look!
Your project looks great and if you have any questions about anything I wrote, feel free to reach out! Well done!
| } | ||
|
|
||
| # Dictionary assigning point values to the letters | ||
| LETTERS_VALUE = { |
There was a problem hiding this comment.
Both of these global dictionaries look great!
|
|
||
| letter_bank.append(random_letter) | ||
|
|
||
| return letter_bank |
There was a problem hiding this comment.
Overall, this function looks good! It's a different approach than most that I've seen so far and it's quite clever! The one thing to point out is that while you are taking into account quantity limits, your code currently treats every letter as if they have the same chance of being picked! In reality though, there are more "A"s than "Q"s so the probability of picking an A would be much higher and your current algorithm doesn't take that into account! I will say this wasn't required and it wasn't a test case, so you are not required to include that, but if you have some extra time and would like to try an implementation that weights the letters, feel free to do so!
|
|
||
|
|
||
| # Count occurrences of a letter in an iterable | ||
| def count_items(item, iterable): |
There was a problem hiding this comment.
I love the way you built your own count method here! It is a builtin method, but being able to write it yourself can help you better understand how to use it! Helper functions always encouraged!
| pass | ||
| capital_word = word.upper() | ||
|
|
||
| for letter in set(capital_word): |
There was a problem hiding this comment.
Nice move to loop through a set as opposed to the entire capital word! Good catch to avoid duplicates!
| capital_word = word.upper() | ||
|
|
||
| for letter in set(capital_word): | ||
| if letter not in letter_bank or count_items(letter, capital_word) > count_items( |
| score += LETTERS_VALUE[letter] | ||
|
|
||
| # Add bonus points for words of length 7-10 | ||
| if len(word) >= 7 and len(word) <= 10: |
There was a problem hiding this comment.
This looks good! Remember that we have a slightly more concise way to write a conditional to figure out if a value falls into some range. What might that look like?
|
|
||
|
|
||
| # Function to find the minimum length of words in a list | ||
| def calculate_min_length(list_of_words): |
| current_winner = word | ||
|
|
||
| return current_winner, max_score |
There was a problem hiding this comment.
Overall this function works and passes all the tests, so great job! Specifically, this is a really great way to separate out each section of data and then parse through it one at a time. There is nothing inherently wrong with that and it's a great way to learn and make sure you understand each step that you are doing!
Once challenge I have for you is to remember that when it comes to these sorts of algorithms, it can be super helpful to keep in mind what information you need to store as well as what information you don't! For example, we really only need to hold onto words that could be or are the highest scoring word. We don't really care about any of the other words! The dictionary you create maps every word to a score but we likely won't need most of those scores and the words that map to them.
Your logic is super sound for how to discover which word and score to update and I think you could leverage that logic to limit the number of extra dictionaries and lists that you create here! No need to make the actual change, but it is something to think about moving forward!
|
Thank you, Adrian! I appreciate all your comments! I will update the code with the changes you pointed out just for myself so that I have that practice. |
No description provided.