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

Spruce - Ngozi Amaefule #42

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

Spruce - Ngozi Amaefule #42

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d750ed
completed draw_letters function by randomly choosing a letter and add…
shainabeth Sep 22, 2021
6738500
Wave 02 complete; all tests passed
ngoziamaefule Sep 23, 2021
82e5da0
Completed Wave 2 by adding a scoring system that returns the sum of t…
shainabeth Sep 24, 2021
6a9cedc
Wave 04 Complete; passed all tests. Invoked function from Wave 03 to …
ngoziamaefule Sep 25, 2021
2470532
Refactoring complete
ngoziamaefule Oct 11, 2021
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
143 changes: 139 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,146 @@
import random

def draw_letters():
pass
pool_of_letters = {
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" (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.

'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
}

list_of_letters = list(pool_of_letters.keys())

draw_ten = []




for i in range(10):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than looping 10 times, we could loop until the draw_ten list gets 10 elements (while its length is less than 10). Doing so would allow us to avoid having to write the extra inner loop to find a letter that we still have a tile for. Instead, if the count for the "drawn" letter were 0, we could continue the loop to start again from the top and pick a new letter.

random_letter = random.randint(0, 25)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than hard coding a range of 25, consider calculating this based on the list we'll be using the result in. This would allow the code to automatically work correctly in the event that we changed the set of characters used for tiles without needing to hunt down and update literals (which might not be easy to find).

Here, we could replace 25 with len(list_of_letters) - 1

letter = list_of_letters[random_letter]

while pool_of_letters[letter] == 0:
random_letter = random.randint(0, 25)
letter = list_of_letters[random_letter]

draw_ten.append(letter)
pool_of_letters[letter] -= 1


return draw_ten



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

Choose a reason for hiding this comment

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

Good job making a copy! Otherwise we'd side-effect the letter bank and destroy the letters in it!

if len(word) > len(letter_list):
return False

elif len(word) == len(letter_list):
for letter in word:
if letter in letter_list:
letter_list.remove(letter)
if letter_list is False:
return True
else:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

During the review, I mentioned that I don't think there were any tests in the project that would exercise this condition (all of the words used in the uses_available_letters tests are shorter than the tile count). That being said, this condition could probably be cut entirely, since checking whether a word smaller than the length of tiles, and checking whether a word equal in length to the available tiles are nearly the same, and I'd expect the smaller word logic to work.


else:
whole_word = []
for letter in word:
if letter not in letter_list:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider calculating a frequency map of the letters before entering the for loop here. Building a frequency map is a linear operation, but we could look up the letters in constant time, whereas using not in on the list requires a linear operation for each loop pass.

return False
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice that if a previous branch in a conditional terminates with a return, then a subsequent elif could be written simply as if and an else can be omitted entirely. This is because if the code didn't return, we must have been in the "negative" branch, otherwise we wouldn't have reached this spot in the code (we would have returned!). For the else situation, that can save us a level of indentation and make the code a little easier to read.

whole_word.append(letter)
letter_list.remove(letter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove is a linear operation because 1) it has to loop through the list to find the value to remove, and 2) it has to shuffle any "downstream" values forward to close any gap left after the removal. If instead we operated on a pre-calculated frequency map, you could track whether a letter is present/used up similar to how you enforced the tile pick counts in draw_letters, and each operation would be constant time!

if len(whole_word) == len(word):
return True
else:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to avoid writing code like:

if some_condition:
    return True
else:
    return False

Instead, return the result of the condition directly, so here

return len(whole_word) == len(word)




def score_word(word):
pass
value_1 = ["A", "E", "I", "O", "U", "L", "N", "R", "S", "T"]
value_2 = ["D", "G"]
value_3 = ["B", "C", "M", "P"]
value_4 = ["F", "H", "V", "W", "Y"]
value_5 = ["K"]
value_8 = ["J", "X"]
value_10 = ["Q", "Z"]

sum = 0

if len(word) > 6:
sum += 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could totally ternary this!

sum = 8 if len(word) > 6 else 0

Also, we could also introduce some constants to avoid the bare literals, and we could try to avoid using sum as a variable name (since it's also the name of a global built-in python function). That could look like

LONG_WORD_BONUS = 8
LONG_WORD_BONUS_LENGTH = 7
BASIC_SCORE = 0

score = LONG_WORD_BONUS if len(word) >= LONG_WORD_BONUS_LENGTH else BASIC_SCORE

It may be a little longer, but it describes all the parts of the calculation.


for letter in word.upper():
if letter in value_1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking each letter in a range like this is definitely more maintainable than checking for each letter one by one, but consider using a dictionary to map each possible letter to a score. We could look up the score for each letter, and add it to our overall score!

Building the table is a little painful (and redundant), but we could imagine starting from a table that uses a string of all the letters which share a score a the key, and the score as the value. That would be very maintainable and have no redundancy. Then we could write code to convert from that structure to a dictionary where each letter is its own key. This would let us have a maintainable data representation, AND quick score lookups!

sum += 1
elif letter in value_2:
sum += 2
elif letter in value_3:
sum += 3
elif letter in value_4:
sum += 4
elif letter in value_5:
sum += 5
elif letter in value_8:
sum += 8
elif letter in value_10:
sum += 10

return sum



def get_highest_word_score(word_list):
pass
score_dict = {}
max_score = 0
high_score_words = {}

for word in word_list:
score_dict[word] = score_word(word)

for score in score_dict.values():
if score > max_score:
max_score = score

for word, score in score_dict.items():
if score == max_score:
high_score_words[word] = score

shortest_word_length = 10

for word, score in high_score_words.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting steps of finding all the scores for the words, finding the max score, finding all the words that have the max score. Then bailing out as soon as we find one with a length of 10, otherwise finding the length of the shortest word with the max score, then finding the first word with that shortest length. It's a little complex (some comments would help call out what you're doing along the way), but the complexity is still linear overall, placing this in the same complexity class as the most optimal possible solution. (We can't do any better than linear with respect to the number of words, since we need to inspect/compare the score of every word in the list!)

Notice that your use of dictionaries here works because dictionaries preserve their insertion order for iteration. We usually talk about dictionaries as being unordered, and it's tru that the key values have no ordered relationship, but since you built up the various dictionaries by iteration, we can depend on the order of the dictionary items to be the same as the order of that first word list!

if len(word) == 10:
return word, score

elif len(word) < shortest_word_length:
shortest_word_length = len(word)

for word, score in high_score_words.items():
if len(word) == shortest_word_length:
return word, score