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.

Sign up for GitHub

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
+94 −5
Open

Snow Leopards- Ja H. #84

Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4fa7d81
Completed wave 1. Defined draw_letters function
lavazane Nov 3, 2022
c9beec1
Defined uses available letters function and completed wave 2 of project.
lavazane Nov 3, 2022
c2aad30
define score_word function. Completed wave 3 of project
lavazane Nov 3, 2022
bdadb86
completed wave 4 by creating get_highest_word_score function
lavazane Nov 3, 2022
05fd20b
added comment to practice branching
lavazane Mar 3, 2023
3d0b406
added name
lavazane Mar 3, 2023
2cc2e6c
Merge pull request #1 from jahopkins/new_feature
jahopkins Mar 3, 2023
5e4a9ad
delete changes
lavazane Mar 3, 2023
a9574f2
added comment
lavazane Mar 3, 2023
bbfc83b
Merge pull request #2 from jahopkins/another_practice_branch
jahopkins Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 92 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,99 @@
import random

def draw_letters():
pass

letter_distribution = {
Copy link

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.

Copy link

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,
        ...
    )

'A': 9,
'B': 2,
'C': 2,
'D': 4,
'E': 12,
'F': 2,
'G': 3,
'H': 2,
'I': 9,
'J': 1,
'K': 1,
'L': 4,
'M': 2,
'N': 6,
'O': 8,
'P': 2,
'Q': 1,
'R': 6,
'S': 4,
'T': 6,
'U': 4,
'V': 2,
'W': 2,
'X': 1,
'Y': 2,
'Z': 1
}
ten_rand_letters = []

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

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.

Copy link

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.


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

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.

return ten_rand_letters

def uses_available_letters(word, letter_bank):
pass

upper_word = word.upper()

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

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).

return False
else:
continue
Comment on lines +49 to +50
Copy link

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.

return True

def score_word(word):
pass

score = 0
letter_values = {
("A", "E", "I", "O", "U", "L", "N", "R", "S", "T") : 1,
("D", "G") : 2,
("B", "C", "M", "P") : 3,
("F", "H", "V", "W", "Y") :4,
("K") : 5,
("J", "X") : 8,
("Q", "Z") : 10
}

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

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.

score += letter_values[key]

if 7 <= len(word) <= 10:
score += 8

return score

def get_highest_word_score(word_list):
pass

highest_scoring_word = word_list[0]
highest_score = score_word(highest_scoring_word)

for word in word_list:
word_score = score_word(word)
if word_score < highest_score:
continue
Comment on lines +84 to +85
Copy link

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:

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

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!

if len(highest_scoring_word) == 10:
continue
elif len(word) == 10:
highest_scoring_word = word
highest_score = word_score
elif len(word) < len(highest_scoring_word):
highest_scoring_word = word
highest_score = word_score

return (highest_scoring_word, highest_score)