Skip to content

Phoenix -- Beenish Ali #30

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Phoenix -- Beenish Ali #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
131 changes: 127 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

It looks like there was only a single commit for the whole project. Try to commit more frequently on future projects. After finishing up each wave is a great time to commit your work so far.

Original file line number Diff line number Diff line change
@@ -1,11 +1,134 @@
import random
import sys
Copy link

Choose a reason for hiding this comment

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

It looks like you imported sys to get the maxsize. That works, and since you're not grabbing a system helper function, doesn't violate the spirit of what we asked around imports, but there's an approach for locating something that's "the shortest" which doesn't require this. Check my feedback down in get_highest_word_score for more on this.


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
}

SCORE_CHART = {
Copy link

Choose a reason for hiding this comment

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

👍 Nice decision to make the score table another global CONSTANT. And using a dictionary gives us a very efficient way to look up the scores. I also like that you listed the letters alphabetically, as that helps a reader quickly understand that all of the letters do properly have a score, and it's easy to find a specific letter if we need to make any changes.

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


def draw_letters():
pass
drawn_letters = []
letters = list(LETTER_POOL.keys())
used_letters = {}
while len(drawn_letters) < 10:
Copy link

Choose a reason for hiding this comment

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

Since each loop might not result in adding an additional letter to the hand, we don't know exactly how many times the loop will run. Running until our hand has the desired number of tiles will make sure the loop runs as many times as necessary.

We could also consider giving a name to 10, like HAND_SIZE so that it's clear what this number represents here. For a small project like this, we probably know what all the numbers represent, but for larger systems, having unnamed numbers showing up all over the place makes the code more difficult to understand.

random_letter_pos = random.randint(0,25)
Copy link

Choose a reason for hiding this comment

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

Try to give names to any numbers appearing here so that we know what they represent, or calculate them from some other relevant piece of data. For example, we can calculate the top of the randint from the length of letters. This would make it easier to maintain our code if we needed to work with different numbers of tiles (maybe we need characters from another language, or we allow certain punctuation). By tying the random range to the list we'll be using the index in, that's one less bit of data that could get out of sync if we make changes to our code.

A subtle point to notice also is that with this approach (picking a letter, and then seeing whether we have enough to use), has a slightly different probability of picking various letters. You'll never pick more than the available letters (so you'll never have more than one Z, for example), but your chances of getting a Z at all are higher with this approach. For example, picking the first letter, this approach has a 1 in 26 chance of drawing a Z, but if we were to consider the letters as all being in a big pile of tiles, we'd only have a 1 in 98 (total number of tiles) change of getting a Z. Effectively, this means we have a much higher chance of drawing "difficult" letters with this approach.

random_letter = letters[random_letter_pos]
letter_count = LETTER_POOL[random_letter]
if random_letter in used_letters:
used_count = used_letters[random_letter]
if used_count < letter_count:
Copy link

Choose a reason for hiding this comment

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

Notice that because we have to check the number of the picked letter used so far, we aren't guaranteed to successfully pick a letter each time through the loop. Though unlikely, it's possible we could get "stuck" picking letters we've used up already, never finishing picking a hand (or at least taking longer than expected). Even when there's a random process involved, we like to write logic that is guaranteed to make progress, rather than logic that could behave unpredictably.

Think about some approaches that we could use to ensure that we definitely pick a letter each time we iterate.

drawn_letters.append(random_letter)
used_letters[random_letter] = used_count + 1

else:
drawn_letters.append(random_letter)
used_letters[random_letter] = 1
return drawn_letters


def uses_available_letters(word, letter_bank):
pass
letter_bank_copy = letter_bank[:]
Copy link

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:
letter = letter.upper()
Copy link

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.

if letter in letter_bank_copy:
letter_bank_copy.remove(letter)
else:
return False
Comment on lines +87 to +90
Copy link

Choose a reason for hiding this comment

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

Personally, I would reverse the sense of this comparison so that we can unindent the logic a little.

        if letter not in letter_bank_copy:
            return False    

        letter_bank_copy.remove(letter)

Taking into account my feedback about using a dictionary below, the way we check for a letter, and "use" it would differ, but structuring the logic this way would still work.

Copy link

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
score = 0
for letter in word:
letter = letter.upper()
score += SCORE_CHART[letter]
Comment on lines +97 to +99
Copy link

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.

if len(word) >=7 and len(word) < 11:
score += 8
Comment on lines +100 to +101
Copy link

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.

return score

def which_came_first(original_list, num, possible_winners):
Copy link

Choose a reason for hiding this comment

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

Nice helper function to enforce the ordering of the original word list. Interestingly, even though this may seem necessary, since we used a dictionary in the middle of processing the words, it turns out to not be the case. Even though dictionaries don't store their keys ordered relative to the keys themselves, they do maintain the order they are inserted into the dictionary, meaning that all of the operations that were done by iterating over the words, even after using the dictionary, still preserve the original word order. With the suggested changes below, this helper could be removed entirely.

for word in original_list:
if len(word) == num:
for possible_winner in possible_winners:
if word == possible_winner[0]:
return possible_winner



def get_highest_word_score(word_list):
pass
words_and_scores = {}
for word in word_list:
word_score = score_word(word)
words_and_scores[word] = word_score
scores = list(words_and_scores.values())
max_score = max(scores)
Copy link

Choose a reason for hiding this comment

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

I would have prefered seeing logic written out to find the maximum score, rather than using the max function. Try to hold the perspective that writing code yourself is good practice for thinking about how we need to break down steps for a computer to follow. Ideally, if you really wanted a function like max, it would have been great to see you write your own helper function to do this. Even writing code again that we've written before is good practice as long as we actually write it again rather than referring back to a previous implementation.

possible_winners = []
for word, score in words_and_scores.items():
if score == max_score:
possible_winners.append((word, score))
Comment on lines +121 to +123
Copy link

Choose a reason for hiding this comment

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

We could chunk up a lot of these sub-problems into helper functions. For instance, finding the max score, and building the list of winner candidates (words that have the max score) could fit very nicely into their own helper functions.

Comment on lines +121 to +123
Copy link

Choose a reason for hiding this comment

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

Rather than storing tuples here (which means we have to keep indexing into the tuples to get the word or score), we can observe that once we have found the max score, every candidate winner will have that max score. So we could wait to build the necessary tuple until just as we're returning, and only work with a list of words while figuring out the winner.

if len(possible_winners) == 1:
return possible_winners[0]
min = sys.maxsize # setting the min variable to a very high number
Copy link

Choose a reason for hiding this comment

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

Rather than pulling in a special value, like maxsize, a common way to start finding a min/max is to just assume that the first candidate value is the min/max, and then look for other values that beat it.

for possible_winner in possible_winners:
if len(possible_winner[0]) == 10:
return which_came_first(word_list, 10, possible_winners)
Copy link

Choose a reason for hiding this comment

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

In fact, since all of the possible_winners are tied for the max score, as soon as we find a 10 letter word, we can return directly, without the which_came_first helper. This is because, while we don't usually think of a dictionary as being ordered, they do preserve the order the keys were inserted. So as we're iterating over the keys to extract the possibel winners, the original word order is preserved!

            return possible_winner

if len(possible_winner[0]) < min:
min = len(possible_winner[0])
Copy link

Choose a reason for hiding this comment

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

If we also kept track of which word had the new min, then when we got to the end, w would be able to return that word directly. As above, the word order is preserved, even though we went through a dictionary (which we often think of as unordered) in the middle. Something like

            min = len(possible_winner[0])
            min_word = possible_winner

then later

    return min_word

return(which_came_first(word_list, min, possible_winners))
Copy link

Choose a reason for hiding this comment

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

Nit: return isn't a function, so don't include parentheses unless we need to wrap a line. Even then, leave a space between return and the parens so that it looks less like a function call.



3 changes: 2 additions & 1 deletion main.py
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Watch out for commiting unintentional changes. If git status shows a file with changes that we don't want to include (such as with a following git add .), we can drop the changes with git restore path/to/file, so here git restore main.py.

Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,5 @@ def main(wave):
wave = int(args[1])
else:
wave = "ERROR"
main(wave)
main(wave)