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

Sapphire-Niambi #123

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

Sapphire-Niambi #123

wants to merge 5 commits into from

Conversation

Lemmi-C
Copy link

@Lemmi-C Lemmi-C commented Mar 24, 2023

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.

Hi Niambi! Your code passes all the wave 2 tests and 3/4 of the wave 1 tests. It looks like you still have some more work to do for wave 3 and 4 so I'll have to give you a 🔴 grade.

Please work with your 1:1 on next steps for finishing this project.

In this PR, you'll see comments from me on how to DRY up your code. In addition, I see that you only made 4 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. Detailed commit messages are super helpful for future bug hunting!

Otherwise, keep up the good work✨

adagrams/game.py Outdated
Comment on lines 4 to 10
def draw_letters():
pass

letter_pool = ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"]

#Returns an array of 10 random uppercase letters in the appropriate quantities

letters = random.choices(letter_pool, weights = [9, 2, 2, 4, 12, 2, 3, 2, 9, 1, 1, 4, 2, 6, 8, 2, 1, 6, 4, 6, 4, 2, 2, 1, 2, 1], k = 10)

Choose a reason for hiding this comment

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

👍 Nice work considering the statistical probability of pulling each letter into a hand!

Choose a reason for hiding this comment

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

letter_pool is a list we aren't making changes to so we can consider it a constant variable and use the all uppercase naming convention to illustrate that. LETTER_POOL.

adagrams/game.py Outdated
Comment on lines 16 to 35
letter_dict = {}
word_dict = {}
#returns either True or False
#check if letter in submitted word are in letter_bank (some or all letters in hand)
# returns true if every letter in submitted word is available(in the right numbers) in letter_bank
#print(letter_bank)

for item in letter_bank:
if (item in letter_dict):
letter_dict[item] += 1
else:
letter_dict[item] = 1

#print(letter_dict)

def score_word(word):
pass
for elem in word.upper():
if (elem in word_dict):
word_dict[elem] += 1
else:
word_dict[elem] = 1

Choose a reason for hiding this comment

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

Nice use of frequency maps ✨ !

Comment on lines +40 to +43
for key, value in word_dict.items():
if key not in letter_dict or not value <= letter_dict[key]:
return False
return True

Choose a reason for hiding this comment

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

👍

adagrams/game.py Outdated
Comment on lines 48 to 75
letter_score_dict = {
"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.

Optionally, we could declutter this function by moving this dictionary outside and declaring it as a constant variable.

adagrams/game.py Outdated
#for each letter in the word:
for char in word.upper():
score_total += letter_score_dict[char]
word_length = len(word.upper())

Choose a reason for hiding this comment

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

The word length will be the same regardless of the casing so we can omit upper.

adagrams/game.py Outdated
for word in word_list:
#looks at word_list and calc tie breakers
if len(word) == wo

Choose a reason for hiding this comment

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

This line of code makes the program crash. Before submitting PR's, we should check if any of our code is causing the program to crash before submitting. This is ensure that we aren't sharing buggy code that could affect our teams codebase and workflows.

adagrams/game.py Outdated
return best_word[0][1]
#if there is a tie: the word with fewer letters wins unless one word had exactly 10 letters
if len(word) in word_list == 10:

Choose a reason for hiding this comment

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

For unfinished conditionals, functions, and loops we can insert the keyword pass as a placeholder so that our code doesn't crash.

Suggested change
if len(word) in word_list == 10:
if len(word) in word_list == 10:
pass

return False
return True

def score_word(word):

Choose a reason for hiding this comment

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

Looks like you're failing all the tests for this function.

elif word_length == "":
word_length = 0
return score_total

def get_highest_word_score(word_list):

Choose a reason for hiding this comment

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

Looks like you still have more work to do for this function!

… to pass wave three tests, and updated get_highest_word_score to pass wave 4 tests
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.

2 participants