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

Ada C19 Amethyst Kaliane V. #125

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

MotifNoticer
Copy link

No description provided.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Awesome job Kaliane ✨

Your submission covers all the learning goals and passes all the tests! You have earned a 🟢 grade!

In your PR, you'll find comments on ways to refactor and DRY up your code. I see that you only made 3 commits. For future projects, please make your commits small, frequent, and with detailed commit messages! Your commit messages should describe what changes you've added to your code. For example, "added score_word logic and passed the tests". Detailed commit messages are super helpful for future bug hunting!

Otherwise, keep up the great work Kaliane! 💜✨

Comment on lines +3 to +59
LETTER_POOL = {
'A': 9,
'B': 2,
'C': 2,
'D': 4,
'E': 12,
'F': 2,
'G': 3,
'H': 2,
'I': 9,
'J': 1,
'K': 1,
'L': 4,
'M': 2,
'N': 6,
'O': 8,
'P': 2,
'Q': 1,
'R': 6,
'S': 4,
'T': 6,
'U': 4,
'V': 2,
'W': 2,
'X': 1,
'Y': 2,
'Z': 1
}

SCORE_CHART = {
'A': 1,
'B': 3,
'C': 3,
'D': 2,
'E': 1,
'F': 4,
'G': 2,
'H': 4,
'I': 1,
'J': 8,
'K': 5,
'L': 1,
'M': 3,
'N': 1,
'O': 1,
'P': 3,
'Q': 10,
'R': 1,
'S': 1,
'T': 1,
'U': 1,
'V': 4,
'W': 4,
'X': 8,
'Y': 4,
'Z': 10
}

Choose a reason for hiding this comment

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

Nice work using the uppercase naming convention to indicate that this is a constant variable ✨

Since LETTER_POOL is accessed by one function, it would be reasonable to place them inside the functions rather than as a constant. There are tradeoffs, the structures would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.

Comment on lines 61 to +70
def draw_letters():
pass
letters = []
pool = LETTER_POOL.copy()
while len(letters) < 10:
hand = random.choice(list(pool))
if pool[hand] <= 0:
continue
pool[hand] -= 1
letters.append(hand)
return letters

Choose a reason for hiding this comment

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

Looks like you made ✨Hard Mode✨! Using list on a dictionary ( LETTER_POOL) creates a list of the dictionary's keys. In this case, you're making a list of all 26 letters with an equal probability of being randomly chosen via random.choice. So in other words, (from the README.md):

Since there are 12 Es but only 1 Z, it should be 12 times as likely for the user to draw an E as a Z.

The tests do not focus on the statistical accuracy of your code. Instead, the tests check for outcomes that should NOT be possible such as having more than 1 Z in our hand.

Hard Mode is a totally valid way of making this project so don't fret about resubmitting or redoing this (if you have time, you definitely can but it's low-priority compared to the other Learn, PSE, and projects you also need to focus on).


def uses_available_letters(word, letter_bank):
pass
bank = letter_bank.copy()

Choose a reason for hiding this comment

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

Nice use of a copy to avoid side effects on letter_bank.

Comment on lines +74 to +80
for character in word:
character = character.upper()
if character in bank:
bank.remove(character)
else:
return False
return True

Choose a reason for hiding this comment

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

We can refactor this slightly by using upper on the word and checking for falsiness first to exit the function early:

    for character in word.upper():
        if character not in bank:
            return False
        bank.remove(character)
    return True          

Comment on lines 82 to +89
def score_word(word):
pass
score = 0
for character in word:
character = character.upper()
score += SCORE_CHART[character]
if len(word) >= 7:
score += 8
return score

Choose a reason for hiding this comment

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

👍 Nice work! This is a really clean solution ✨

@@ -0,0 +1,287 @@
{

Choose a reason for hiding this comment

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

Did you mean to add this file to your PR? If not, definitely be wary of what you're adding to your commits!

candidates = [(w, score_word(w)) for w in word_list]

Choose a reason for hiding this comment

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

🔥 Beautiful use of a list comprehension to build a list of tuples!!

Comment on lines +93 to +100
ties = [c for c in candidates if c[1] == highest_score[1]]
if len(ties) == 1:
return ties[0]
for c in candidates:
if len(c[0]) == 10:
return c
return min(ties, key=lambda t: len(t[0]))

Choose a reason for hiding this comment

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

🔥 Great use of lambda, list comprehension, and min !

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