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 - Jesse P. #49

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

Maple - Jesse P. #49

wants to merge 10 commits into from

Conversation

jessepope
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.

Andrea and Jesse, nicely done! Your code is easily readable! My main suggestion is shortening your conditionals in get_highest_word_score and remove those that have no actionable commands nested inside

def draw_letters():
pass
letters_left = LETTER_POOL.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 glad to see a copy() so you don't produce side effects

pass
letters_left = LETTER_POOL.copy()
choice_ten = []
while len(choice_ten) < 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 like the while loop! It's six to half a dozen of the other using a for-loop instead, but I think this makes more sense readability-wise

Comment on lines +65 to +69
letter = random.choice(list(letters_left))
if letters_left[letter] > 0:
choice_ten.append(letter)
letters_left[letter] -= 1
return choice_ten
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 spotted that you need to check if there are still letters available after randomly selecting a key from random.choice() since it could still be picked, but have 0 availability.


def uses_available_letters(word, letter_bank):
pass
def uses_available_letters(word, choice_ten):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

letters_in_hand.remove(letter)
else:
return False
return True

def score_word(word):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines +96 to +112
pass
elif score > top_score:
top_word = word
top_score = score
elif score == top_score:
if len(word) == len(top_word):
pass
elif len(top_word) == 10:
pass
elif len(word) == 10:
top_word = word
top_score = score
elif len(word) < len(top_word):
top_word = word
top_score = score
return top_word, top_score
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 works great, you can also consider removing any if statements that don't have any actionable commands inside of it, like line 102 and 104.

If it's because it's easier to read, awesome! totally valid! you are likely to see these if statements stripped out, though to keep the code drier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 nice tuple return!

Comment on lines +113 to +116



Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@@ -1,11 +1,117 @@
import random

LETTER_POOL = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

:+1 good job making this a constant variable with all caps

Comment on lines +70 to +71


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

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