Skip to content

Sphinx - Leidy Martinez#44

Open
Leidy-Martinez wants to merge 7 commits into
Ada-C22:mainfrom
Leidy-Martinez:main
Open

Sphinx - Leidy Martinez#44
Leidy-Martinez wants to merge 7 commits into
Ada-C22:mainfrom
Leidy-Martinez:main

Conversation

@Leidy-Martinez

Copy link
Copy Markdown

No description provided.

Comment thread adagrams/game.py Outdated
Comment on lines +35 to +36
letter_pool_copy = LETTER_POOL.copy()
letters_list = list(letter_pool_copy.keys())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work here, love to see you using the methods available on objects!

Comment thread adagrams/game.py Outdated

while len(draw) < 10:
#Take a random number within the len of letter pool and get the letter in that index
random_letter = letters_list[random.randint(0,len(letter_pool_copy)-1)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right now your code isn't actually utilizing the weight distribution of the letters. Currently, all letters have the same chance of being selected (they all can only appear their respective limits). randint is only randomly selecting from a pool of integers from 0 to 26. This means that Z is just as likely as likely to be selected as E. You would need it to select from a pool of integers from 0 to 98 to have the correct distribution. How could we change your letter pool to consider this weighted distribution?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you!! It looks like I forgot my statistics classes. Fixed! 🎉

Comment thread adagrams/game.py Outdated
random_letter = letters_list[random.randint(0,len(letter_pool_copy)-1)]

#Check if the letter is still availabe in the letter pool and add it to the hand
if letter_pool_copy[random_letter] >= 1:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A more pythonic way of writing this line would be:

if letter_pool_copy[random_letter]:

It works the same as what you have since if letter_pool_copy[random_letter] is 0 then it will be a falsy value.

@Leidy-Martinez Leidy-Martinez Sep 20, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Noted! 📝

Comment thread adagrams/game.py Outdated
#check if the word is an anagram of some or all of the given letters in the hand

letter_bank_copy = letter_bank.copy()
word = word.upper()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work on ensuring that all the letters where of the same casing. You could also use casefold, which is used for caseless matching. You can find more information on the method here.

Also, be mindful of re-assigning the parameter of a function. That could cause issues for the caller of the function if the change is not expected. Usually we want to avoid this unless specifically told to.

Comment thread adagrams/game.py Outdated
if len(word) >= 7:
total_points += 8

return total_points

@mikellewade mikellewade Sep 20, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is the alternative approach we were talking about in co-working:

Click this to see code.
SCORE_DICT = {
    'a': 1, 'e': 1, 'i': 1, 'o': 1, 'u': 1, 'l': 1, 'n': 1, 'r': 1, 
    's': 1, 't': 1, 'd': 2, 'g': 2, 'b': 3, 'c': 3, 'm': 3, 'p': 3, 
    'f': 4, 'h': 4, 'v': 4, 'w': 4, 'y': 4, 'k': 5, 'j': 8, 'x': 8, 
    'q': 10, 'z': 10
}

def score_word(word):
score = 0

casefold_word = word.casefold()

for letter in casefold_word:
    print (letter)
    score += SCORE_DICT.get(letter)

if len(casefold_word) > 6:
    score += 8

return score

EDIT: I forgot to casefold the keys in the dictionary.

Comment thread adagrams/game.py
and len(highest_word) != 10:
highest_word = word

return (highest_word, highest_score) No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work using that debugger and getting this code working! You are a debugger pro! 🪲

Comment thread adagrams/game.py Outdated
highest_score = current_score

#If a tie and no word is 10 char, pick the shortest word
if current_score == highest_score \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would change this to an elif so that this line of code and the other elif statement below don't run when line 136 is not True. Right now both of these conditions are still checked when though they will never run if current_score > highest_score. It's inconsequential here, but could cause some logical errors in other cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That makes a lot of sense!! Thank you so much for your feedback, very much appreciated 🤝

Comment thread adagrams/game.py Outdated
# if len(word) >= 7:
# points += 8

# return points

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm sure this is for refactoring reasoning, but don't forget to remove comments like this when pushing up your code. For projects and things it is fine though. More so for industry.

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