Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

olenarostotskyy
Copy link

No description provided.

@olenarostotskyy olenarostotskyy changed the title Olena Rostotskyy Adagram project c17 otter class Olena Rostotskyy and Joanna Parisi Adagram project c17 otter class Mar 31, 2022
Copy link

@jbieniosek jbieniosek left a 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

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.

Comment on lines +72 to +73
pool = deepcopy(letter_bank)
pool = [p.upper() for p in pool] # convert each element in a list to uppercase

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:

Suggested change
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())

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"]

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.

Comment on lines +152 to +158
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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment on lines +161 to +166
if len(e) >=10:
highest_score_word=e
break
elif len(e)<len(highest_score_word):
highest_score_word=e

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])

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.

Suggested change
result=tuple([highest_score_word,highest_score])
result=tuple(highest_score_word,highest_score)

Comment on lines +56 to +58
# while count <v:
# pool.append(k)
# count+=1

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.

Comment on lines +64 to +68
# 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants