Kunzite - Allie S. #122
Kunzite - Allie S. #122
Conversation
There was a problem hiding this comment.
Nicely done, Allie! You'll find some feedback below, but overall you did a wonderful job! Congrats on finishing your first project!
Things to consider:
- Comments can be very helpful to other programmers or yourself if you come back to code months later. However, be careful with comments for every line. It can make reading your code more difficult. Instead, use docstrings for each function to organize your comments into one spot. Then you can explain the function's utility without breaking up the code. Here are some examples from PEP257 styling guide.
| letter_list = [] | ||
|
|
||
| # While the length of letter_list list is not 10, continue finding an available letter | ||
| while len(letter_list) != 10: |
There was a problem hiding this comment.
Consider using while len(letter_list) < 10. This condition casts a wider net of catching issues than only stopping when len(letter_list) reaches 10. A bug could be introduced upon refactoring, and then jump past 10 into 11 or 12.
| while len(letter_list) != 10: | ||
| # convert the keys in the LETTER_POOL dictionary into a list | ||
| # randomly choose a letter form this list | ||
| letter = random.choice(list(LETTER_POOL.keys())) |
There was a problem hiding this comment.
Nice idea! One thing to consider: should each letter have the same probability of being chosen, though? For example, should "Z" have a probability of 1 in 26 or 1 in 98 (the total amount of tiles that can be drawn)?
How could we represent that in a data structure? Maybe a list of all tiles and their duplicates (ie, ['A','A','A','B','B'] and so on) to randomly choose from.
A more advanced solution might be using the random.choice method, but with more than one param. Here's an example of how you can "weigh" your string or list: https://www.geeksforgeeks.org/how-to-get-weighted-random-choice-in-python/
| for letter in word.upper(): | ||
| # checks if the current letter if found in the letter_bank_copy list | ||
| if letter in letter_bank_copy: | ||
| # if the current letter is found, remove that instance from the list | ||
| letter_bank_copy.remove(letter) |
There was a problem hiding this comment.
This works fine! We've only just talked about Big O and time/space complexity, but one thing to notice here is we have a 3 nested for loops, even if we haven't explicitly stated for___ in ___ three times.
The remove method loops through letter_bank_copy and if letter in letter_bank_copy loops under the hood. Nothing wrong with 2 nested for loops. At times they are the easier solution. But! we have 3 here, and a dictionary of letters counted from the letter_bank would help us here.
Same with remove. It must find the letter, remove it, then shift any values over to fill the space.
How might we create a "frequency map" (a dictionary of items counted) with a constant lookup (O(1)) of letters found in letter_bank, then use it to check if all those letters are in the word?
| for key, value in SCORE_CHART.items(): | ||
| # iterate through each letter in capitalized word | ||
| for letter in word.upper(): | ||
| # searches for each letter in a key of letters from SCORE_CHART | ||
| if letter in key: |
There was a problem hiding this comment.
Here again we've got 3 nested loops. Let's refactor the SCORE_CHART dictionary so that each letter is its own key. Is there some overhead setting that up? Yes. But it would also let us look up a letter's score in "constant" time (letter in score_dict) rather than iterating through an entire string, which is "linear" time (letter in "AEIOULNRST").
Again, we only just discussed time complexity and Big O, but that's a brief explanation as to why a dictionary is a beneficial data structure we can use to hold data that needs to be frequently looked up.
| best_word = best_word | ||
| highest_score = highest_score |
There was a problem hiding this comment.
Hm, do we need this conditional? We are re-assigning the current highest scored word with itself. Let's get rid of it.
| best_word = best_word | ||
| highest_score = highest_score |
There was a problem hiding this comment.
Again, we can get rid of this conditional altogether because the current highest word is already assigned to the best_word variable.
No description provided.