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

Maple - Elly Wong #34

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

Maple - Elly Wong #34

wants to merge 10 commits into from

Conversation

cecilia-yyw
Copy link

No description provided.

Copy link
Collaborator

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Elly and Ro, nicely done! My one suggestion is potentially shortening the helper function names, so it is more easily read inside the max and min functions.

pass
import random

def draw_letters(): # Time complexity: O(1) Space complexity: O(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow! you added the space/time complexities! very cool!

Comment on lines +31 to +37
for i in range (10):
random_letter = random.choice(list_letters_pool)
list_letters_drawn.append(random_letter)
# remove the drawn letter from the original list of letter pool after
# each random draw
list_letters_pool.remove(random_letter)
return list_letters_drawn
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! another way you could do this is with a while loop! run the loop until the length of list_letters_drawn is 10

Comment on lines +58 to +62
if each_char in letter_bank_dict and char_count <= \
letter_bank_dict[each_char]:
continue
else:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

while this is valid, continue statements aren't commonly used. We can dry this up by doing something like:

Suggested change
if each_char in letter_bank_dict and char_count <= \
letter_bank_dict[each_char]:
continue
else:
return False
if each_char not in letter_bank_dict or char_count > \
letter_bank_dict[each_char]:
return False

"Z" : 10,
}
score = 0
for each_char in word.upper(): # unify the case of each char
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 good idea

# Time complexity: O(n) Space complexity: O(1)
dict_word_score = {}
for each_word in word_list:
dict_word_score[each_word] = score_word(each_word)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 yaaay helper function

min_length = min((length_of_each_word_in_list_word_with_max_score(list_word_with_max_score)))
return helper_func_return_tuple_with_word_max_score(dict_word_score, min_length, max_score)

def helper_func_return_tuple_with_word_max_score(dict_word_score, length, max_score): # max or min length
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 love a good helper function

if len(each_word) == length:
return (each_word, max_score)

def length_of_each_word_in_list_word_with_max_score(list_word_with_max_score):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 glad you came up with some ideas on how to make your get_highest_word_score function stick to the solid principles

list_letters_pool.remove(random_letter)
return list_letters_drawn

def uses_available_letters(word, letter_bank):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

score += 8
return score

def get_highest_word_score(word_list):
Copy link
Collaborator

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