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

Spruce Class - Veronica Osei #41

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

Conversation

VeronicaO27
Copy link

No description provided.

Copy link
Collaborator

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Nice job!

All waves are complete and all tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. You used lists and dictionaries throughout, and were aware of when you needed to make copies to avoid side-effects. Really nice uses of docstrings as well! Be sure to check out PEP257 for some general docstring guidelines, and this article has some links to other recommendations.

All in all, this was a really concise approach throughout. I liked seeing the use of helper data structures to improve the efficiency of working with lists of tiles/words. Don't forget to dig into documentation from time to time to recall methods you may have forgotten (get from dict) or to learn about additional capabilities (counts for sample).

Overall, great work!

@@ -1,11 +1,165 @@

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.

I also like having this defined as a "constant" outside the function. Theoretically, we could write our draw_letters method to accept a pool as a parameter (defaulting to this one). But moving a large chunk of data out from a function also just helps declutter the function. 🙂

Comment on lines +44 to +46
for letter, frequency in LETTER_POOL.items():
for i in range(frequency):
letter_list.append(letter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're doing a ranged for where we don't use the loop variable, _ is a good name which communicates to other devs that we don't care about this variable.

    for letter, frequency in LETTER_POOL.items():
            for _ in range(frequency):
                letter_list.append(letter)

But here, we could even use list repetition to avoid the inner loop entirely!

    for letter, frequency in LETTER_POOL.items():
        letter_list += [letter] * frequency

for letter, frequency in LETTER_POOL.items():
for i in range(frequency):
letter_list.append(letter)
return random.sample(letter_list, 10) # chose to use Random.sample for random sampling without replacement.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can even get sample to take care of building the expanded list for us (as of 3.9). In fact, internally, it even avoids building that expanded list!

    return random.sample(LETTER_POOL.keys(), counts=LETTER_POOL.values(), k=10)

When we run across helpful functions, it can be worth looking into the documentation to see whether it has any other functionality that might be useful!

letter_list.append(letter)
return random.sample(letter_list, 10) # chose to use Random.sample for random sampling without replacement.



def uses_available_letters(word, letter_bank):
Copy link
Collaborator

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 tiles to efficiently check whether we can make the supplied word!

Comment on lines +70 to +73
if letter in letter_frequency:
letter_frequency[letter] += 1
else:
letter_frequency[letter] = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use the get method of dict to simplify this a bit:

        count = letter_frequency.get(letter, 0)  # returns 0 if the key isn't in the dict
        letter_frequency[letter] = count + 1

We could also think about using collections Counter (though there's nothing wrong with building the map ourselves. It's great practice!)

Comment on lines +76 to +81
if letter not in letter_bank:
return False
else:
letter_frequency[letter] -= 1
if letter_frequency[letter] < 0:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

get can help us again here!

        if not letter_frequency.get(letter, 0):
            return False
       
        letter_frequency[letter] -= 1

"""
score_of_points = 0

score_chart = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice lookup table for the tile scores. As with LETTER_POOL, we might move this to a "constant" outside the function so that it doesn't need to be re-created each time we call the scoring function.

Comment on lines +130 to +131
if len(word) in range(7, 11):
score_of_points += 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider creating constants to represent these literal values, for example:

MIN_BONUS_LENGTH = 7
MAX_BONUS_LENGTH = 11
LENGTH_BONUS = 8

Also, using in on range has to check each generated value in the range to see whether it matches the input (O(n) over the size of the range). It's not a big range here, but generally, to check whether a value is within a "range" of numbers, we can simply use comparisons, such as:

    # note this in-between syntax is very python-specific. most other languages DON'T work this way
    if MIN_BONUS_LENGTH <= len(word) < MAX_BONUS_LENGTH:
        score_of_points += LENGTH_BONUS

Also, do we really need to check for the upper bound?

    if MIN_BONUS_LENGTH <= len(word):
        score_of_points += LENGTH_BONUS

Comment on lines +147 to +148
highest_score = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice approach of tracking the current winning word and score!

Comment on lines +156 to +163
continue
elif len(winning_word) == 10:
continue
elif len(word) == 10:
winning_word = word
elif len(word) < len(winning_word):
winning_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.

Nice set of tie-breaker checks. There are a number of ways to group these. In fact, it's even possible to combine these into a single check! Try playing around with it!

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