Phoenix - Ajar Duishembieva #26
Conversation
There was a problem hiding this comment.
First off, congratulations on completing your first Ada project, Ajar! You should be extremely proud of what you've accomplished! As a first pass, I'll be scoring your project as a 🟡 Yellow🟡! This means that there were a couple of issues, but overall it passes and you are not required to make any changes.
That being said, if you would like to make changes so I can update it to a Green, you are more than welcome to! There are two small fixes that will boost you to that level here, both in the draw_letters() function:
- As mentioned, the random.choice function works here, but we did ask to avoid using it if possible. See if you can change it to leverage the random.randomint function instead.
- Similarly, your function does not allow for duplicate letters to exist in a hand and would allow the user to draw more than 10 tiles. Feel free to fix this too!
As always, if you have any questions, please don't hesitate to reach out! 😊
| 'X': 1, | ||
| 'Y': 2, | ||
| 'Z': 1 | ||
| } |
There was a problem hiding this comment.
This is a great dictionary to hold the letters and the count of each letter! When creating it, you've followed the naming convention of a global variable which I agree would be the best way to hold this information just in case we need to use this dictionary elsewhere! The one change I would make here would be to move it outside this function which is what we would do to make sure the dictionary is available to the other functions that might need it. If we have several pieces of this kind of information, it is also possible to put your data in a separate file and import it in as well!
| } | ||
| my_dictionary = {} | ||
| my_list = [] | ||
| while len(my_dictionary) != 10: |
There was a problem hiding this comment.
To go along with my previous comment, it's important to remember how length for a dictionary works. We have multiples of many letters in our dictionary. Later on, you account for this by making sure the count of a specific letter in your hand doesn't go past the count of the letter in LETTER_POOL. The problem in this while loop conditional comes in the fact that len(dict) returns the number of key, value pairs in the dictionary. Therefore, the while loop will only stop when you have 10 different letters in the hand, even if you have duplicates. As a result, you may end up with a dictionary that has technically selected 11+ letters.
| my_dictionary = {} | ||
| my_list = [] | ||
| while len(my_dictionary) != 10: | ||
| letter = random.choice(string.ascii_uppercase) |
There was a problem hiding this comment.
I like the way you found a way to get a string of all uppercase letters here. I wonder if there is a way to approach this randomness without using the random.choice() function though? Would it be possible to leverage randomint instead? The other thing to think about here is the fact that right now, you are treating every letter with equal length. There are certain letters that would be more likely to be pulled based off how many are in the pool. How could you use the dictionary to create a collection of every letter rather than just selecting from a collection of 26?
| 'Y': 2, | ||
| 'Z': 1 | ||
| } | ||
| my_dictionary = {} |
There was a problem hiding this comment.
I see that you have two collections to hold the hand of cards that gets picked. As far as the rest of this function goes, you then store what gets picked in the dictionary and then move everything to the list to be returned. With this in mind, is the dictionary actually necessary? What if we just placed everything in the list to begin with?
| else: | ||
| my_dictionary[letter] = 1 | ||
|
|
||
| for key in my_dictionary: |
There was a problem hiding this comment.
With this for loop, you are only looping through the keys and adding them to the list. This unfortunately doesn't account for multiples of a letter that may have been selected.
| if letter in LETTER_SCORES: | ||
| value = LETTER_SCORES[letter] | ||
| total_points += value | ||
| if len(word) == 7 or len(word) == 8 or len(word) == 9 or len(word) == 10: |
There was a problem hiding this comment.
This looks good, but it does start to get a little wordy! Is there a way we could achieve the same result with a compound conditional?
| LETTER_SCORES = { |
There was a problem hiding this comment.
As mentioned before, if we're going to use this same dictionary as before, throw it outside the functions!
| key = letter.upper() | ||
| current_sum = current_sum + LETTER_SCORES[key] | ||
|
|
||
| if len(current_word) == 10: | ||
| current_sum = current_sum + 8 |
There was a problem hiding this comment.
This is code we've already seen in a different function... Is there a way for us to leverage the code that we have already written to achieve the same result?
| word_result[0] = current_word | ||
|
|
||
| return tuple(word_result) |
There was a problem hiding this comment.
It is possible to return a tuple without casting it from another data type! If we have two variables, we can return them both in one statement. For example, if I had two variables called highest_word and highest_score, I could use
`return highest_word, highest_score`
to return them as a tuple.
| previous_word = word_result[0] | ||
|
|
||
| if previous_sum < current_sum: | ||
| word_result[0] = current_word | ||
| word_result[1] = current_sum | ||
| elif previous_sum == current_sum: | ||
| previous_size = len(previous_word) | ||
| current_size = len(current_word) | ||
|
|
||
| if(current_size==10 and previous_size !=10): | ||
| word_result[0] = current_word | ||
| elif current_size!=10 and previous_size !=10 and current_size < previous_size: | ||
| word_result[0] = current_word |
There was a problem hiding this comment.
The logic here looks great! The main area for improvement I see is that you add new variables at each level of nesting. There is nothing inherently wrong with that, but it can start to get difficult to read and understand. In the future, see if there are ways you can reuse variables you've already declared!
No description provided.