Skip to content
Open
Changes from all commits
Commits
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
135 changes: 131 additions & 4 deletions adagrams/game.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎉 Nice job doing commits regularly as you finished each wave. A commit after each wave is a good target to aim for with projects. When working on your own code, it will be up to you to decide when it's a good time to commit, but the more we practice now, the more we'll be in the habit of committing. Otherwise, it's easy to forget.

Original file line number Diff line number Diff line change
@@ -1,11 +1,138 @@
import random

LETTER_POOL = {
'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
}

def draw_letters():
pass
letters_in_hand = []
letters_list = []
count = 0

for letter in LETTER_POOL:
for value in range(LETTER_POOL[letter]):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Notice that we don't use the variable value in the loop body. Python does require that we provide a loop variable name here, but if we're not intending to actually use it, a common variable name is just the underscore character _. This tells the reader of our code that the only reason that variable is there is because Python requires it, not because we're doing anything with it.

This is most common in for _ in range(some_value): loops, where we just need to run the code in the block some_value times, without needing to know which iteration we're on while doing it.

letters_list.append(letter)
Comment on lines +37 to +39

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This code does a nice job of building up a list of all the available tiles. We could make this a little clearer by moving the logic to a helper function and giving it a good descriptive name, maybe build_letter_list.



while count < 10:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To loop 10 times, it's recommended to use a for loop, like

    for _ in range(10):

since there's no danger of forgetting to initialize count, or updating count in the loop.

Alternatively, we can phrase this as

    while len(letters_in_hand) < 10:

which links the number of iterations to the work we actually need to do in the loop. We need to be building up the list of letters in our hand as we're looping (that's the whole point), so tying the loop condition to the work we need to do helps the reader focus on the important part of the loop.

random_letter_index = random.randint(0, len(letters_list) - 1)
letters_in_hand.append(letters_list[random_letter_index])
letters_list.pop(random_letter_index)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Using pop (rather than remove) lets us extract precisely the letter that was randomly picked. However, we'll see that popping from an aribtrary location has the same performance as using remove (it is related to the length of the list). But if we know exactly which position we want to pop, and the order of things in the list doesn't really matter, we can use a trick to pop from an aribrary location in a way that doesn't depend on the length of the list. We can swap the value we want to pop to the end of the list, then pop from the end (popping from the end doesn't depend on the length of the list). Consider something like

        last_pos = len(letters_list) - 1
        letters_list[last_pos], letters_list[random_letter_index] = letters_list[random_letter_index], letters_list[last_pos]
        letters_list.pop()

count += 1


return letters_in_hand


def uses_available_letters(word, letter_bank):
pass

word_upper = word.upper()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Necessary for the case insensitive comparison. Because we're using English letters, upper case is fine. But for a more general notion of case-insensitive, take a look at casefold.


letter_bank_copy = letter_bank[:]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 This is a very idiomatic way to copy a list in python. We need a copy due to the approach of removing letters from the bank, which would destroy the player's hand if we didn't use a copy.


for letter in word_upper:
if letter not in letter_bank_copy:
return False
else:
letter_bank_copy.remove(letter)
Comment on lines +59 to +62

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since the if block terminates the immediate logic by returning, we can un-nest the main logic (it doesn't need to be under an else). else ensure that only one of the two blocks under an if/else is actually executed, by splitting the blocks into code that runs when the if conditional is true, and code that runs when it's false. Normally, if code that should only be run when the condition is false gets left unindented, it would get run regardless of the condition value

        if some_condition:
            # only runs when condition is true
            some_logic()

        # runs whether the condition is true or false
        some_other_logic()

However, here, when the condition is true, we exit the function, meaning the only way we could reach the code after the condition is if the condition had been false . If it had been true, we would have exited the function!

        if letter not in letter_bank_copy:
            return False
            
        letter_bank_copy.remove(letter)

This may be a little confusing, but it's a very common pattern (and allows us to indent less in python), so it's important to get sued to this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The performance of both checking whether something is in a list, and the remove function depends on the length of the list we are looking at. And since we are doing these operations in a loop, it compounds the cost. We'll hear more about this as we work on Big O material, but we often want to minimize performance costs in code. One way to acheive that here would be to iterate over the hand once to build a dictionary holding a count of how many times each letter appears. As we process each letter in the word, we still need to look it up in the dictionary and check the count, but these are more efficient in a dictionary than in a list.


return True


def score_word(word):
pass
word_upper = word.upper()
score = 0

if len(word_upper) >= 7 and len(word_upper) <= 10:
score += 8
Comment on lines +71 to +72

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We can also write this to make it more clear why we're doing this. Consider giving names to the "magic numbers" (hard coded literal values that appear in code) that are used here. We could use MIN_BONUS_LEN for 7 (may not even be necessary to check the upper length), and LENGTH_BONUS for 8. We could even move this to a helper function called something like add_bonus_points.


score_chart_dict = {}
score_chart_dict["A"] = 1
score_chart_dict["E"] = 1
score_chart_dict["I"] = 1
score_chart_dict["O"] = 1
score_chart_dict["U"] = 1
score_chart_dict["L"] = 1
score_chart_dict["N"] = 1
score_chart_dict["R"] = 1
score_chart_dict["S"] = 1
score_chart_dict["T"] = 1
score_chart_dict["D"] = 2
score_chart_dict["G"] = 2
score_chart_dict["B"] = 3
score_chart_dict["C"] = 3
score_chart_dict["M"] = 3
score_chart_dict["P"] = 3
score_chart_dict["F"] = 4
score_chart_dict["H"] = 4
score_chart_dict["V"] = 4
score_chart_dict["W"] = 4
score_chart_dict["Y"] = 4
score_chart_dict["K"] = 5
score_chart_dict["J"] = 8
score_chart_dict["X"] = 8
score_chart_dict["Q"] = 10
score_chart_dict["Z"] = 10
Comment on lines +74 to +100

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider moving the score chart to a global constant dictionary, defined as a literal (just like you did for LETTER_POOL. Removing it from this function makes the function easier to read, and using literal syntax has less chance of errror.

Also, consider listing the letters alphabetically (rather than grouped by score). As a reader of the code, if I want to convince myself that all letters are accounted for, that's easier if they are listed alphabetically. That can calso make it easier if I need to update the scores at some point.


for letter in word_upper:
score += score_chart_dict[letter]
Comment on lines +102 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Great job calculating the base word score by summing the scores of the individual letters.


return score


def get_highest_word_score(word_list):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great job of breaking this process down into a series of steps that leads us to finding the winning word. While it's possible to do this "all at once" in a single loop, the "time complexity" (we'll start talking about this for Big O soon) is identical to what we have here, even though there appears to be more looping.

Because you've broken things down into a sequence of distinct steps, we could make this even more self-documenting by moving each step into a helper function with a descriptive name. Consider find_max_score, find_tied_winning_words, find_first_ten_letter_word, etc...

Then reading the function calls would more or less be describing the overall logic of the function rather than needing to go line-by-line, section-by-section, reminding ourselves, "oh yeah, this part is finding the list of words tied for the winning score".

pass
max_score = 0
win_word_list = []

for word in word_list:
score = score_word(word)

if score > max_score:
max_score = score
win_word_list.clear()
win_word_list.append(word)
Comment on lines +117 to +118

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Alternatively

            win_word_list = [word]

elif score == max_score:
win_word_list.append(word)

if len(win_word_list) == 1:
return win_word_list[0], max_score

for win_word in win_word_list:
if len(win_word) == 10:
return win_word, max_score

min_length = 10
win_word_with_min_length = ""

for win_word in win_word_list:
win_word_length = len(win_word)
if win_word_length < min_length:
min_length = win_word_length
win_word_with_min_length = win_word
Comment on lines +132 to +136

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Nice approach to find the shortest word, which among the words tied for the highest score, as long as there are no 10 letter words (checked earlier) the shortest word is the winner.


return win_word_with_min_length, max_score