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 - Cecily M. and Angela F. #28

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

Conversation

fan-gela
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.

The main thing I'd recommend thinking about, especially when working primarily with lists, is whether we can do a little pre-calculation here and there to build helper structures (often a frequency map of some kind stored in a dict) to improve the efficiency of our code. This is also true for data representation. Sometimes the most optimal way to store some data isn't the most optimal way to perform our needed lookups. And intermediate data transformation can give us the best of both worlds!

Overall, great work!

def draw_letters():
pass
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.

Consider declaring this as a "constant" (maybe POOL_OF_LETTERS) outside this function. Doing so wouldn't really save on runtime (since we need to modify the counts, we'd need to make a copy at the start of each call to the method), but it does declutter the method a bit by allow us to move the large data structure to some innocuous corner of our file.

Comment on lines +36 to +38
for key in letter_pool:
for idx in range(letter_pool[key]):
letter_pool_list.append(key)
Copy link
Collaborator

@anselrognlie anselrognlie Oct 6, 2021

Choose a reason for hiding this comment

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

This code is building a list of expanded letters to be used for random picking later. From a purely syntax perspective, keep in mind that if we know that we'll need both the key and the value (letter_pool[key] here), we can iterate using the .items() helper to get both at once. Further, we can use list repetition to avoid the inner loop. Combined, this would look something like:

    for letter, count in letter_pool.items():
        letter_pool_list += [letter] * count

The alternative approach would be to make a copy of the letter count data, and pick random letters, decrementing their available counts. The initial copy (required so that the next draw has all the letters available again) is roughly equivalent in time to building the extra letter list here. The upside would be that each letter pick would be roughly O(1) rather than O(n) for operations like the choice and remove operations.

for idx in range(letter_pool[key]):
letter_pool_list.append(key)

while len(letters_drawn) != 10:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the logic here guarantees that a random letter will be successfully drawn each time through the loop, we could use a for loop with a range expression to run this 10 times. Other approaches (such as the decrement approach I mentioned above) might have loops where a valid letter isn't chosen (if the count is 0), in which case while loop based on the accumulated length of the selected tile list is also a great approach. But here, we could do:

    for _ in range(10):

The use of _ as the variable name is usually a signal to developers that we don't care about this variable (it isn't used), but one is required for syntax reasons.


def uses_available_letters(word, letter_bank):
pass

letters_copy = letter_bank.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Since the approach used to check whether the word can be made from the letter bank is destructive to the hand, this copy is necessary (otherwise we'd be destroying the player's hand anytime we check for word validity!).

Another approach would be to build a frequency map out of the letter bank, and then check whether each letter in word can be accounted for in the map. This would have the upside that each letter check would be O(1), while checking for in in a list (and the subsequent remove) is O(n) for each letter.

Comment on lines +52 to +59
while word_length > 0:
for letter in word:
if letter in letters_copy:
letters_copy.remove(letter)
word_length -= 1
else:
raise ValueError
return True
Copy link
Collaborator

@anselrognlie anselrognlie Oct 6, 2021

Choose a reason for hiding this comment

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

Nice use of errors!

Notice, however, that remove itself will raise a ValueError if we attempt to remove an item that isn't in the list, so we don't need to do the in check and raise the error ourselves. We also don't need to track the word length ourselves, since if we exhaust the available letters in letters_copy before finishing iterating over the letters in word, we'll end up in the error handler, but if there were enough letters to satisfy the word, we'll reach the end of the word and return True. Something like the following:

        for letter in word:
            letters_copy.remove(letter)
        return True


def score_word(word):
pass

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 in draw_letters, 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 +99 to +100
if len(word) >= 7 and len(word) <= 10:
total_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 = 10
LENGTH_BONUS = 8

Something else to consider, is it even necessary to check whether the length of the word is less than or equal to 10?


highest_scoring_word = ("",0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider keeping these as 2 distinct variables during the calculation, and wait to return them together until the end of the method. By working with a tuple directly here, we need to keep track of which position in the tuple holds the score, and which holds the word.

Instead, we could make 2 variables: highest_scoring_word and highest_score, so then we would be very clear about which value we were working with. Then, at the end, we could return highest_scoring_word, highest_score.

Comment on lines +113 to +114
or len(highest_scoring_word[0]) >= 10:
Copy link
Collaborator

Choose a reason for hiding this comment

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

PEP8 recommends that we avoid explicit line breaks (\) and instead prefer implicit line breaks (any expression surrounded by parentheses can pretty much be wrapped anywhere). For example:

            if (len(word) == len(highest_scoring_word[0])
                or len(highest_scoring_word[0]) >= 10):

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