-
Notifications
You must be signed in to change notification settings - Fork 69
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
Olena Rostotskyy and Joanna Parisi Adagram project c17 otter class #35
base: master
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.
Great work Olena and Joanna! Very clean, readable code. Nice work with the frequent commits. This project is green.
if len(letter_list) < 10: | ||
letter_list.append(letter) | ||
|
||
return letter_list |
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.
This function will pass the tests, but the letter distribution is not correct. letter_key_list
will contain all of the letters only once, and then the loop starting on line 41 will pick the first 10. Essentially, this is a letter distribution of 1 tile per letter.
pool = deepcopy(letter_bank) | ||
pool = [p.upper() for p in pool] # convert each element in a list to uppercase |
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.
Both of these lines are creating a new list. They can be combined:
pool = deepcopy(letter_bank) | |
pool = [p.upper() for p in pool] # convert each element in a list to uppercase | |
pool = [p.upper() for p in letter_bank] # convert each element in a list to uppercase |
In addition, a deep copy is not necessary, a shallow copy, ie letter_bank.copy()
would work here as well to create a copy of the list. The letters in the letter bank are immutable strings. A deep copy is only needed when the contents of the list are mutable, and the original data needs to be preserved.
if letter.upper() not in pool: | ||
return False | ||
else: | ||
pool.remove(letter.upper()) |
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.
👍
list_4 = ["F", "H", "V", "W", "Y"] | ||
list_5 = ["K"] | ||
list_6 = ["J", "X"] | ||
list_7 = ["Q", "Z"] |
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.
See feedback in Adagrams PSE for notes on optimization.
if v>highest_score: | ||
highest_score=v | ||
highest_score_words.clear() | ||
highest_score_words.append(k) | ||
elif v==highest_score: | ||
highest_score_words.append(k) |
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.
Nice!
if len(e) >=10: | ||
highest_score_word=e | ||
break | ||
elif len(e)<len(highest_score_word): | ||
highest_score_word=e |
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.
Fantastic work with this algorithm! Small note, I recommend avoiding single letter variable names - more descriptive variable improve readability.
highest_score_word=e | ||
|
||
result=tuple([highest_score_word,highest_score]) |
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.
The tuple here can be built directly, no need to create a list and then convert it to a tuple.
result=tuple([highest_score_word,highest_score]) | |
result=tuple(highest_score_word,highest_score) |
# while count <v: | ||
# pool.append(k) | ||
# count+=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.
This would work to create a letter pool with the correct distribution.
# while counter <10:# creating random index from 0 to 10 | ||
# random_inex=random.randint(0, len(pool)-1) | ||
# hand.append(pool[random_inex]) #adding randomly picked letter to our hand (list of 10) | ||
# pool.pop(random_inex) #removing the same element from the pool | ||
# counter+=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.
👍
No description provided.