-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
modified functions to pass all tests #111
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🟢 Green!
I have some nitpicks, comments, and suggestions, but those don't detract from the fact that this is a solid submission. Very nice!
Some overall feedback:
- The tests are passing, and your code even passes a corner case I noticed during grading that aren't in the tests.
- I recommend committing as you finish parts of waves. It can be easy to mess up previously-implemented functions as you work on new behavior, so it's valuable to have your history saved in
git
. - You can remove the pseudocode comments you started from; I find it makes your code more readable.
random.choices
doesn't give you exactly the right statistical result, which is what you had to work around; you wantrandom.sample
for choosing without replacement.
#Goal: returns array/list of 10 strings | ||
|
||
#build letter pool in the form of a dictionary | ||
letter_pool = {"A":9, "B":2, "C":2, "D":4, "E":12, "F":2, "G":3, "H":2, "I":9, "J":1, "K":1, "L":4,"M":2, "N":6, "O":8, "P":2, "Q":1, "R":6, "S":4, "T":6, "U":4, "V":2, "W":2, "X":1, "Y":2,"Z":1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] When a dictionary gets big (26 entries big, for example), it's better to split it across multiple lines. Please also put a space after :
. If you stick to a single line, also put a space after ,
and around {
and }
.
letter_pool = {
"A": 9,
"B": 2,
# ...
"Z": 1
}
#loop thru a for-loop 10 times to get 10 letters from letter pool: | ||
#adapted from https://stackoverflow.com/questions/69119270/randomly-selecting-a-word-from-a-dictionary-with-a-given-probability | ||
for i in range(10): | ||
#draw each letter randomly based on each letter's probability with replacement | ||
next_letter = random.choices(list(letter_pool.keys()),weights=list(letter_pool.values()),k = 1) [0] | ||
#add each letter drawn to hand list | ||
hand.append(next_letter) | ||
#print(hand) #verify | ||
#if the frequency of the next letter drawn exceeds the frequency of that letter in the letter_pool, then... | ||
if hand.count(next_letter) > letter_pool[next_letter]: | ||
#...remove that letter from both the letter_pool and hand so it can't be drawn again | ||
letter_pool.pop(next_letter) | ||
hand.remove(next_letter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job doing the research and documenting what you've found! It looks like you had to work around the fact that random.choices
chooses with replacement. You had to remove each letter as you chose it. random.sample
does what you were looking for:
hand = random.sample(list(letter_pool.keys()), counts = list(letter_pool.values()), k = 10)
That said, thinking about how we can implement this functionality ourselves can help us gain insight into what that might be. It also helps us decide what these functions might do to our big-O complexity.
Remember that not every language has the same built-in functions, so we should be able to implement this same functionality ourselves. In the projects, we don’t ask for any features that we don’t think you should be able to write yourself. For drawing a random hand, the only external function that would be “required” is a way to pick a random number (such as randint). At this stage in our coding journeys, it’s often more valuable to re-implement things by hand to practice thinking about how to break down and approach the problem than to use a single library function to solve the problem for us.
pass | ||
#Goal: returns array/list of 10 strings | ||
|
||
#build letter pool in the form of a dictionary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] space and capitalize comments
# Build ...
for char in word: | ||
#return False for the word if the frequency of character in word is > the frequency of | ||
#that same character in letter_bank | ||
if word.count(char) > letter_bank.count(char): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using built-in functions like count
, you'll want to consider their time complexity. What's the big-O complexity of this built-in function? (You can search for it on stack overflow, but you can logically conclude a lower bound of its time complexity by thinking about what it must do in order to function correctly.) Given the big-O of count
, what does it do to the big-O of your overall function? Think about the function as-is, then also consider what might happen if requirements changed, for example, if we were required to draw an arbitrary number of letters.
Also, after you think about that, it's okay to note that analysis in a comment and then conclude that the performance of count
is just fine for this implementation given the current, real-world constraints. 😄
#loop thru each character in word | ||
for char in word: | ||
#loop thru each key in score_chart dict | ||
for key in score_chart: | ||
#if length of key is > 1. In other words, if there's > 1 letter for a point value... | ||
if len(key) > 1: | ||
#...then loop thru each letter in said key | ||
for letter in key: | ||
#if character in word matches said letter... | ||
if char == letter: | ||
#...then add corresponding point value to word's score | ||
score += score_chart[key] | ||
#if length of key is 1. In other words, if there's only 1 letter for a point value... | ||
else: | ||
#...and if character in word matches said letter... | ||
if char == key: | ||
#...then add corresponding point value to word's score | ||
score += score_chart[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[comment] Checking each letter in a range like this is definitely more maintainable than checking for each letter one by one. It could be faster, though. Consider how many checks this code does if char
happens to be "Z"
. How many more than "A"
? If performance became a concern for this code, you could consider using a dictionary to map each possible letter to a score.
Building that dictionary is a little painful (and redundant), but we could imagine starting from a dictionary that uses a string of all the letters which share a score as the key, and the score as the value. That would be very maintainable and have no redundancy. Then we could write code to convert from that structure to a dictionary where each letter is its own key. This would let us have a maintainable data representation, AND quick score lookups!
That might look like starting with your dictionary of tuples and adding a function to build a per-letter dictionary of scores...
score_chart = {
("A", "E", "I", "O", "U", "L", "N", "R", "S", "T"): 1,
...
("Q", "Z"): 10
}
def build_letter_to_score():
""" produces a dictionary of letter to score, e.g. { "A": 1, "E": 1, ... "Z": 10 } """
...
You will often find that, on the job, this kind of extra effort is worth the development time to balance performance and maintainability.
#turn all characters in word into upper case like those in letter_bank | ||
word = word.upper() | ||
#build score chart as a dictionary | ||
score_chart = {("A", "E", "I", "O", "U", "L", "N", "R", "S", "T"):1, ("D","G"):2, ("B", "C", "M", "P"):3, ("F", "H", "V", "W", "Y"):4, "K":5, ("J", "X"):8, ("Q", "Z"):10} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[idea] Since str
s are collections of characters and support in
, you can save some hassle by using strings instead of tuples here:
score_chart = {
"AEIOULNRST": 1,
...
"QZ": 10
}
and changing your loop to do:
for char in word:
for key, key_score in score_chart.items():
if char in key:
score += key_score
break
Using a string as a collection of characters is an occasionally-useful trick. It keeps you from having to deal with the single vs. multi-character case, and let you use in
. Although, see below for a more efficient idea.
highest_score = word_score_dict[string_with_highest_score] | ||
#print(highest_score) #verify | ||
#loop thru dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are good, and sometimes you can have too much of a good thing! In most of this function, your comments are almost exactly the same as the lines of code they document. We avoid that in industry because the lines of code may change, but devs aren't always careful about changing the comments to match. It's also more for humans to read, but doesn't give them extra information.
What comments might you write to describe what this code is doing without describing how it does it (since that is what's most likely to change later)?
#if score is equal to the highest score, then add word from dict to winning_words_list to get list of highest-scoring words | ||
if value == highest_score: | ||
winning_words_list.append(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] Since you're comfortable with list comprehension below, this could be done similarly
winning_words_list = [key for key, value in word_score_dict.items() if value == highest_score]
if winning_word_with_ten_letters != None: | ||
#print(winning_word_with_ten_letters) #verify | ||
return winning_word_with_ten_letters, score_word(winning_word_with_ten_letters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] You already calculated the score, so you can reuse it
return winning_word_with_ten_letters, score_word(winning_word_with_ten_letters) | |
return winning_word_with_ten_letters, word_score_dict[winning_word_with_ten_letters] |
shortest_word = (min(winning_words_list, key=len)) | ||
#get length of this shortest word to determine what the smallest number of letters is | ||
shortest_word_len = len(shortest_word) | ||
#look thru list, find the first word w/ the smallest number of letters | ||
winning_word = next(word for word in winning_words_list if len(word) == shortest_word_len) | ||
#print(winning_word) #verify | ||
#return first word w/ smallest number of letters along w/ score | ||
return winning_word, score_word(winning_word) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] You'd probably find that if you rewrote this whole function from scratch and begin with calculating "max" using a traditional for loop, as opposed to relying on max
and next
, you'd end up with a simpler to read and reason about, but still correct, result. Trying to shoehorn in usage of built-in functions often comes back to bite me when I solve these code challenges. I also find myself recommending to folks that they should practice the non-built-in methods for interview practice, to get comfortable with cases when they can't get the built-in functions to do exactly what they need.
Seattle Cohort 19
Anh Huynh