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

Cedar - Rae and Laurel #33

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

Conversation

paperbackwriter2
Copy link

Project submission

Copy link
Collaborator

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

Nice work! It's great that you used this opportunity to explore OOP and refactor. Great work using a branch for this refactored work. Your code is clear and logical. I've left a few inlines comments on ways you might consider refactoring, but overall, great work!

Comment on lines +74 to +75
if remaining_letters_count >= 10:
# do this 10 times:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider whether these if is necessary. I supposed if we had less than 10 letters in the LETTER_POOL this would help us avoid an index error, but we may want to deal with that edge case in a different way such as

if len(LETTER_POOL) < 10:
    return "LETTER_POOL is too small"

or maybe not -- just something to consider :)

# add letter to list
chosen_letters.append(drawn_letter[0])
# decrease letter pool count by 1
LETTER_POOL[drawn_letter[0]] -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make a copy of the LETTER_POOL so that our function does not directly modify this constant.

if not isinstance(word, str) and len(word) <= 10:
return False
# if word is valid
for char in word.upper():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clear, logical code!

pass
# checks to make sure that word is composed of elements in letter_bank
letter_bank_copy = letter_bank[:]
if not isinstance(word, str) and len(word) <= 10:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not quite sure what this check is doing. A comment to explain this validation check would enhance readability.

high_score_words = [word for word in list_of_word_objects if word.score == high_score]
if len(high_score_words) > 1:
return tie_breaker(high_score_words, high_score)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work encapsulating this functionality in a helper function.

return word.word, score
# otherwise return shortest word
winning_word = min(word_list, key= lambda w: w.length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Comment on lines +111 to +112
high_score_words = [word for word in list_of_word_objects if word.score == high_score]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clever and concise!

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