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

Snow Leopards- Ja H. #84

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

Snow Leopards- Ja H. #84

wants to merge 10 commits into from

Conversation

jahopkins
Copy link

I did not have much time to go back and refactor.

Copy link

@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 your tests are passing and your code is laid out well overall. Nice use of using some built-ins, and I like how much you handled with your own approaches. From a strict statistical perspective, your draw_hands doesn't quite follow the distribution of available tiles, but you are still producing valid hands. The hands might just be a little more challenging! And nice job with how clean your get_highest_word_score approach was!

def draw_letters():
pass

letter_distribution = {

Choose a reason for hiding this comment

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

Since you use a non-destructive method to draw your hand, this definition of letter_distribution could be moved outside the function to be global (to the module). This allows us to focus on the logic within the function, rather than having the body of the function be dominated by the data definition. Alternatively, we could condense writing this (maybe multiple tiles per line) but that would have an impact on the readability of this data.

Choose a reason for hiding this comment

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

When using strings as the dictionary keys, I like to the use the dict() function-style of making a dictionary. It uses keyword argument calling to let us set string keys without needing to quote them. For example

    letter_distribution = dict(
        A=9,
        B=2,
        C=2,
        ...
    )

ten_rand_letters = []

while len(ten_rand_letters) < 10:
random_letter =random.choice(list(letter_distribution.keys()))

Choose a reason for hiding this comment

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

As a minor optimization, the keys from the distribution could be stored in a list before the loop to avoid having to iterate through the dict each time through this loop.

Choose a reason for hiding this comment

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

Strictly speaking, choosing from the availaable letters gives us a 1 in 26 chance of choosing any particular letter. However, according to the tile counts, we should have only a 1 in 98 chance of picking a Z. You do have code to ensure that you don't pick more than the possible number of any letter, but you're still much more likely to get some of the lower frequency, harder to use letters in your hand.

An approach that respects the frequencies of the tiles could involve building a list where each tile is represented the number of times indicated in the letter_distribution , and making a choice from that list. We would also then need to adjust the remaining number of tiles so that the percentages adjust as we pick tiles.

Also, think about what choice is doing. Could we implement it's logic ourselves? Reimplementing library routines are a great way to practice for interview questions.

Comment on lines +38 to +39
if ten_rand_letters.count(random_letter) < letter_distribution[random_letter]:
ten_rand_letters.append(random_letter)

Choose a reason for hiding this comment

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

This code correctly prevents us from picking more of a letter than is available in distribution table, but it does require iterating over the hand being built each time. In our specific case, the hand is small, but in a more general case, the hand might be of arbitrary size, so avoiding iterating over it while in an outer loop would be desirable. One way we could avoid doing so would be to track an additional dict that stores the used count for each letter (we can check/update that in constant time), or decrement the count of letters remaining in the distribution (also constant time).

Another point to keep in mind with this approach, is that while it might seem like the outer while loop is going to run 10 ties, because of the random picking and needing to check whether the pick was valid, technically we don't know how many times it will run. "Randomly", Z could get picked over and over, and the hand picking woud get stuck. There are other overall approaches that run in more predictable time modelled a little more directly on thinking about having a pile of tiles and picking randomly from that pile.

Comment on lines +49 to +50
else:
continue

Choose a reason for hiding this comment

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

If the only think under the else is continue, and there's no following code in the loop being skipped as a result of the continue, it would be more usual to leave off the entire else.

upper_word = word.upper()

for letter in upper_word:
if letter not in letter_bank or letter_bank.count(letter) < upper_word.count(letter):

Choose a reason for hiding this comment

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

As above, be careful using count in a loop, since it is also a loop. Again, because of the small size of the hands we're working with, we won't see a performance issue, but in a more general case, we should be aware of whether we have extraneous nested loops.

In this case, we could build a dictionary of the counts of the letters in the word, and another holding the counts of the letters in the hand. Then we could compare the count of each word letter to the count of each hand letter (like here, but we would have counted the word and hand letters only once rather than each time through the loop).

upper_word = word.upper()
for letter in upper_word:
for key in letter_values.keys():
if letter in key:

Choose a reason for hiding this comment

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

By organizing the score table to have a tuple of tiles as the key, we are effectively forced to iterate over the individual values in the score table keys to find our tile. This is not using the benefits of a dictionary to its utmost. When storing data in a dictionary, we should put the data that we expect to frequently lookup by as the key rather than in the value.

If our table stored the letter tile as the key and the score for that tile as the value, we would be able to iterate over the letters in the word, and look up the score for each letter in constant time.

This results in more keys appearing in the table, but for an arbitrary score system, this could still happen with the current structure. If each tile had a distinct score, then we would have as many keys as tiles.

There are always competing concerns between using space efficiently, and runtime performance. But one thing we can definitely say about using dicts is that if we've chosen to use a dict, we should structure the data so that we can look up keys directly.

Comment on lines +84 to +85
continue

Choose a reason for hiding this comment

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

We could leave this condition off, and start witht the second one (as an if)

So start with

        if word_score > highest_score:

highest_scoring_word = word
highest_score = word_score
elif word_score == highest_score and len(highest_scoring_word) != len(word):

Choose a reason for hiding this comment

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

👍 Great job! You captured all of the criteria for breaking ties in a really readable way!

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