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

Sharks - Morgan B. and Philomena K. #55

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

Conversation

Morfiend
Copy link

@Morfiend Morfiend commented Apr 1, 2022

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

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

Great work on this project Morgan and Philomena!

You both passed the tests and met the learning goals. I left a couple of small comments about naming variables/functions and optimizations.

You both have earned a 🟢 on Adagrams!

import random
from collections import Counter

LETTER_POOL = {

Choose a reason for hiding this comment

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

I like that this defined as a "constant" outside the function. Moving a large chunk of data out from a function helps declutter the function.

And I see what you did there with the letter frequency and letter score in the list. You might even consider renaming LETTER_POOL to something like LETTER_FREQ_WITH_SCORES to indicate that this dict holds 2 different kinds of information.


Returns:
returned_letters (list): list of 10 randomly chosen letters
"""

Choose a reason for hiding this comment

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

Nice, succinct docstring here.

returned_letters = []
for letter, number in LETTER_POOL.items():
letter_list += letter * number[0]
for i in range(10):

Choose a reason for hiding this comment

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

You could also write this loop like this while len(letters) < 10:

Comment on lines +74 to +83
"""
Checks if the provided word utilizes letters only found in letter bank

Parameters:
word (str): the played word
letter_bank (list): bank of randomly assigned letters

Returns:
boolean value based on word validity
"""

Choose a reason for hiding this comment

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

👍

Returns:
boolean value based on word validity
"""
letter_dict = Counter(letter_bank)

Choose a reason for hiding this comment

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

Great job building a frequency map from the letters to efficiently check whether we can make the supplied word!


for letter in word.upper():
try:
if letter in letter_dict.keys() and letter_dict[letter] > 0:

Choose a reason for hiding this comment

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

Take care to use key in dict syntax. If you dokey in dict.keys() you'd reproduce O(n) time complexity since under the hood Python creates a list of keys when you call .keys() and the in operator for lists is linear.

if letter in letter_dict and letter_dict[letter] > 0:

except KeyError:
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.

👍

Comment on lines +120 to +134

Analyzes high scores to check for a tie.
Follows game rules for tie breaker:

Ties are judged based off of letter count:

If Their is a 12 letter word present, returns the
first 12 letter word that appears in the list.

For cases without a 12 letter word, returns the
shortest word in the list.

If the shortest words are the same length, return
the one that appears first in the original list.

Choose a reason for hiding this comment

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

There aren't hard and fast rules about writing docstrings. Some devs, myself included, prefer shorter docstrings where possible. I think calling out the input and outputs suffices since I can gather how ties are handled by reading the code. I think that you could leave this first chunk out and I'd still understand what this function does

Comment on lines +160 to +167
word_length = len(word)
if word_length == 10:
return word, top_score
elif not lowest:
lowest = (word, top_score, word_length)
elif word_length < lowest[2]:
lowest = (word, top_score, word_length)

Choose a reason for hiding this comment

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

You could also pull this into a helper method to simplify the logic in this function

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