BumbleBee-Allison #44
Conversation
There was a problem hiding this comment.
As is, this project does receive a red.
I know that the red feels harsh, but please note that all it means is that there are some updates and refactors that are required before I can give the program a passing score!
The main thing to focus on in your updates is to make sure you are passing all the tests. Right now, test number 3 for wave 01 is failing as are tests number 5 and 8 for wave 04. I've detailed in my comments why each test is failing, now it's up to you to figure out how to pass them!
The other refactor that I am going to ask you to do is to manually rewrite the summation within the score_word() function. You currently use the built in sum() function which we asked y'all to avoid. Writing your own summation algorithm allows you to practice both your algorithmic thinking and your python basics.
One of the great parts of using a pull request for feedback is that once you make the changes on your local repo, you can push the changes and I will see them. Just let me know when you do and I'll take a look at them and update the score accordingly!
Overall, this is a very good first attempt that you should absolutely be proud of! Just make those changes and you'll be good to go!
| @@ -1,13 +1,95 @@ | |||
| from random import randint | |||
|
|
|||
| def draw_letters(): | |||
| pass | |||
| # Letter distribution as per the provided table | |||
| LETTER_DISTRIBUTION = { | |||
There was a problem hiding this comment.
You are declaring LETTER_DISTRIBUTION here as a constant by using all capital letters in the variable name.
According to the Pep 8 guidelines, "Constants are usually defined on a module level..." (https://peps.python.org/pep-0008/#constants). Constants are usually written in such a way that they are accessible to all functions within a module. Since LETTER_DISTRIBUTION is declared within the draw_letters() function, it is not accessible to any other function.
I like the move to make LETTER_DISTRIBUTION constant, just move it outside the function to the top of the module to comply with PEP 8 guidelines!
| } | ||
|
|
||
| # Create a pool of letters based on their frequency | ||
| LETTER_POOL = [] |
There was a problem hiding this comment.
In a similar vein, you indicate that LETTER_POOL is a constant by writing it in all capital letters. You then go on to assign it an empty list which means you are likely going to change it later. Even though we technically can modify a constant without Python interfering, it is against best practices to do so. It is pretty unheard of that we will have a constant that is an empty data structure of any sort (list, dictionary, tuple, etc.).
| LETTER_POOL = [] | |
| letter_pool = [] |
| LETTER_POOL = [] | ||
| for letter, count in LETTER_DISTRIBUTION.items(): | ||
| LETTER_POOL.extend([letter] * count) | ||
| """Draws 10 random letters from the letter pool.""" |
There was a problem hiding this comment.
According to PEP 8 documentation, "A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition." If you would like to keep this docstring (and I think that would be a good idea), make sure it's the first line after the function definition!
| for _ in range(10): | ||
| index = randint(0, len(LETTER_POOL) - 1) # Random index within the pool | ||
| hand.append(LETTER_POOL[index]) # Pick the letter at the random index |
There was a problem hiding this comment.
This is a great start to a loop that picks 10 tiles to create a hand! My main concern here is that I don't see any situation where a letter gets removed from the LETTER_POOL after it gets chosen. This means a letter that only occurs once in the distribution ("Z" perhaps) could get chosen multiple times, which is likely the reason one of the tests for wave 01 fails.
🔴 This will need to be fixed in order to receive a passing score for this project.
| for letter, count in LETTER_DISTRIBUTION.items(): | ||
| LETTER_POOL.extend([letter] * count) |
There was a problem hiding this comment.
Great use of the extend method here to account for the different letter distributions we see amongst our letters!
| if 7 <= len(word) <= 10: | ||
| score += 8 |
There was a problem hiding this comment.
Great job of handling a word with 7-10 letters!
| # Iterate through each word in the word list |
There was a problem hiding this comment.
For readability sake, it's a good idea to add some whitespace before starting a loop!
| best_is_ten = False # Whether the best word has 10 letters | |
| # Iterate through each word in the word list | |
| best_is_ten = False # Whether the best word has 10 letters | |
| # Iterate through each word in the word list |
| best_length = current_length | ||
| best_is_ten = current_is_ten | ||
| return (best_word, best_score) |
There was a problem hiding this comment.
Nice job of returning a tuple here. While the parenthesis are nice, they are not required!
| best_word = "" | ||
| best_score = 0 | ||
| best_length = float('inf') # Start with a very large number |
There was a problem hiding this comment.
I'm not entirely sure why we need to start with a best_length of infinity. By virtue of the fact that our loop below will update everything after the first word, we could simply start it at 0 for simplicity's sake.
| if current_is_ten and not best_is_ten: | ||
| # Prefer the word with 10 letters | ||
| best_word = word | ||
| best_score = current_score | ||
| best_length = current_length | ||
| best_is_ten = True | ||
| elif current_length < best_length: | ||
| # Prefer the word with fewer letters | ||
| best_word = word | ||
| best_score = current_score | ||
| best_length = current_length | ||
| best_is_ten = current_is_ten |
There was a problem hiding this comment.
This is a really good start to the logic of figuring out which word scores the best. That being said, two of the tests for wave 04 are failing with this logic. I believe this all comes down to our elif on line 89. According to our tie breaker rules, a word that has 10 letters will always win against a smaller word with the same score. If neither word is 10 letters, then the smaller one wins. For Example:
['JQ' , 'FHQ']
will have 'JQ' as the winner because they both score 18 points, but 'JQ' is shorter. However:
['AAAAAAAAAA', 'BBBBBB']
will have 'AAAAAAAAAA' as the winner because even though they both score 18 points and 'BBBBBB' is shorter, 'AAAAAAAAAA' has 10 letters so it wins by default.
In your logic, the shorter word always wins against a word that has 10 letters.
🔴 This logic will need to be fixed so that it passes all the tests in order to receive a passing score for this project!
No description provided.