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

Cohort_22_Phoenix_Marjana_Khatun #47

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

Cohort_22_Phoenix_Marjana_Khatun #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fde9e73
Done with wave 1
marjanak Nov 21, 2024
2a1eda1
Done with wave 2
marjanak Nov 21, 2024
11b9f4e
Done with wave 3
marjanak Nov 21, 2024
2feb9e5
Done with wave 4
marjanak Nov 22, 2024
47f3b5f
Fixed spacing
marjanak Nov 22, 2024
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
134 changes: 130 additions & 4 deletions adagrams/game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,137 @@
import random

def draw_letters():
pass
list_letters = []
distribution_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.

Nice placement of the distribution_of_letters structure in the function. It's a tradeoff of visually taking up space in the function so we don't need to worry about changes to the structure between function calls.

"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_letters = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a duplicate of the variable we declare on line 4, so one of them should be removed.

while len(list_letters) != 10:
letters = list(distribution_of_letters.keys())
random_letter = random.choice(letters)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this cohort we shared guidance during Unit 1 asking folks to stick to using randint and avoid functions from the random package that do more of the random selection work like sample and choice. How would your implementation change using only random.randint?

value = distribution_of_letters[random_letter]
if value > 0:
distribution_of_letters[random_letter] = value -1
list_letters.append(random_letter)
Comment on lines +36 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a subtle issue with how we're selecting tiles. We are guaranteed to always get a hand of 10, and we are picking from the full alphabet and ensuring we aren't picking too many of a certain tile however, we aren't taking into account the correct probability of grabbing a particular tile.

When we grab a tile, we are picking from the keys of distribution_of_letters. This means we have an equal 1 in 26 chance to grab any letter. However, based on our distributions, we should have many more of some tiles than others, which means we should be more likely to pick those tiles than others.

We can get around this a number of ways; one possibility is creating a list containing all of our possible tiles, then remove each tile from this list as we randomly select them.

return list_letters

def uses_available_letters(word, letter_bank):
pass
word = word.upper()
letters_copy = letter_bank[:]
for letter in word:
if letter in letters_copy:
letters_copy.remove(letter)
Comment on lines +48 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a clear and concise solution, but what is the time complexity like? For each letter in word we may need to make a full loop over letters_copy. If letter exists in letters_copy, the remove method is O(n) in the worst case where we remove the first element and need to shift all elements down.

How could we use frequency maps to reduce our time complexity?

else:
return False
Comment on lines +51 to +52
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice early exit as soon as we find a letter that isn't in the letter_bank!

return True

def score_word(word):
pass
score = {
"A" : 1,
"E" : 1,
"I" : 1,
"O" : 1,
"U" : 1,
"L" : 1,
"N" : 1,
"R" : 1,
"S" : 1,
"T" : 1,
"D" : 2,
"G" : 2,
"B" : 3,
"C" : 3,
"M" : 3,
"P" : 3,
"F" : 4,
"H" : 4,
"V" : 4,
"W" : 4,
"Y" : 4,
"K" : 5,
"J" : 8,
"X" : 8,
"Q" :10,
"Z" :10
}
total_score = 0
word = word.upper()
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case it isn't a big deal, but we typically don't want to overwrite our parameter variables since we lose access to the original value.

for letter in word:
value = score.get(letter, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great use of get to handle non-letter characters like hyphens!

total_score += value
if len(word) >= 7 and len(word) <= 10 :
total_score += 8
return total_score


def get_highest_word_score(word_list):
pass
highest_word = ""
highest_score = 0
for word in word_list:
word_score = score_word(word)
if word_score > highest_score:
highest_score = word_score
highest_word = word
elif word_score == highest_score:
if len(highest_word) == 10:
continue
elif len(word)== 10:
highest_word = word
elif len(word)<len(highest_word):
highest_word = word
Comment on lines +103 to +108
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't take an action inside the if statement and both sides of the elif blocks have the same contents highest_word = word. Repeating code like this is often a sign that we should look at our code to see if there are ways we could combine our statements to reduce repetition.

Thinking about long-term code maintenance, if we have to change that code in the future, it goes quicker and is less error-prone when there is only one line to update, rather than several lines that we need to keep in sync.

One of many options could be to create variables for the boolean statements:

is_new_word_ten = len(word) == 10 and len(highest_word) != 10
is_new_word_smaller = len(word) < len(highest_word) and len(highest_word) != 10
if is_new_word_ten or is_new_word_smaller:
    highest_word = word

return(highest_word,highest_score)
Comment on lines +105 to +109
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminder about spacing around operators and commas to make statements easier to read quickly.

Suggested change
elif len(word)== 10:
highest_word = word
elif len(word)<len(highest_word):
highest_word = word
return(highest_word,highest_score)
elif len(word) == 10:
highest_word = word
elif len(word) < len(highest_word):
highest_word = word
return(highest_word, highest_score)





























8 changes: 4 additions & 4 deletions tests/test_wave_01.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@
'Y': 2,
'Z': 1
}

#@pytest.mark.skip(reason="Temporarily disabled")
def test_draw_letters_draws_ten():
# Arrange/Act
letters = draw_letters()

# Assert
assert len(letters) == 10

#@pytest.mark.skip(reason="Temporarily disabled")
def test_draw_letters_is_list_of_letter_strings():
# Arrange/Act
letters = draw_letters()
Expand All @@ -48,7 +48,7 @@ def test_draw_letters_is_list_of_letter_strings():
for elem in letters:
assert type(elem) == str
assert len(elem) == 1

#@pytest.mark.skip(reason="Temporarily disabled")
def test_letter_not_selected_too_many_times():

for i in range(1000):
Expand All @@ -65,7 +65,7 @@ def test_letter_not_selected_too_many_times():
# Assert
for letter in letters:
assert letter_freq[letter] <= LETTER_POOL[letter]

#@pytest.mark.skip(reason="Temporarily disabled")
def test_draw_letters_returns_different_hands():
# Arrange/Act
hand1 = draw_letters()
Expand Down
5 changes: 5 additions & 0 deletions tests/test_wave_02.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from adagrams.game import uses_available_letters

#@pytest.mark.skip(reason="Temporarily disabled")
def test_uses_available_letters_true_word_in_letter_bank():
# Arrange
letters = ["D", "O", "G", "X", "X", "X", "X", "X", "X", "X"]
Expand All @@ -13,6 +14,7 @@ def test_uses_available_letters_true_word_in_letter_bank():
# Assert
assert is_valid == True

#@pytest.mark.skip(reason="Temporarily disabled")
def test_uses_available_letters_false_word_in_letter_bank():
# Arrange
letters = ["D", "O", "X", "X", "X", "X", "X", "X", "X", "X"]
Expand All @@ -24,6 +26,7 @@ def test_uses_available_letters_false_word_in_letter_bank():
# Assert
assert is_valid == False

#@pytest.mark.skip(reason="Temporarily disabled")
def test_uses_available_letters_false_word_overuses_letter():
# Arrange
letters = ["A", "X", "X", "X", "X", "X", "X", "X", "X", "X"]
Expand All @@ -35,6 +38,7 @@ def test_uses_available_letters_false_word_overuses_letter():
# Assert
assert is_valid == False

#@pytest.mark.skip(reason="Temporarily disabled")
def test_uses_available_letters_does_not_change_letter_bank():
# Arrange
letters = ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J"]
Expand All @@ -48,6 +52,7 @@ def test_uses_available_letters_does_not_change_letter_bank():
assert is_valid == True
assert letters == letters_copy

#@pytest.mark.skip(reason="Temporarily disabled")
def test_uses_available_letters_ignores_case():
# Arrange
letters = ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J"]
Expand Down
6 changes: 5 additions & 1 deletion tests/test_wave_03.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,26 @@

from adagrams.game import score_word

#@pytest.mark.skip(reason="Temporarily disabled")
def test_score_word_accurate():
# Assert
assert score_word("A") == 1
assert score_word("DOG") == 5
assert score_word("WHIMSY") == 17

#@pytest.mark.skip(reason="Temporarily disabled")
def test_score_word_accurate_ignores_case():
# Assert
assert score_word("a") == 1
assert score_word("dog") == 5
assert score_word("wHiMsY") == 17

#@pytest.mark.skip(reason="Temporarily disabled")
def test_score_zero_for_empty():
# Assert
assert score_word("") == 0


#@pytest.mark.skip(reason="Temporarily disabled")
def test_score_extra_points_for_seven_or_longer():
# Assert
assert score_word("XXXXXXX") == 64
Expand Down
10 changes: 10 additions & 0 deletions tests/test_wave_04.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from adagrams.game import score_word, get_highest_word_score

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_score_accurate():
# Arrange
words = ["X", "XX", "XXX", "XXXX"]
Expand All @@ -14,6 +15,7 @@ def test_get_highest_word_score_accurate():
assert best_word[0] == "XXXX"
assert best_word[1] == 32

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_score_accurate_unsorted_list():
# Arrange
words = ["XXX", "XXXX", "XX", "X"]
Expand All @@ -25,6 +27,7 @@ def test_get_highest_word_score_accurate_unsorted_list():
assert best_word[0] == "XXXX"
assert best_word[1] == 32

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_tie_prefers_shorter_word():
# Arrange
words = ["MMMM", "WWW"]
Expand All @@ -38,6 +41,7 @@ def test_get_highest_word_tie_prefers_shorter_word():
assert best_word[0] == "WWW"
assert best_word[1] == 12

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_tie_prefers_shorter_word_unsorted_list():
# Arrange
words = ["WWW", "MMMM"]
Expand All @@ -51,6 +55,7 @@ def test_get_highest_word_tie_prefers_shorter_word_unsorted_list():
assert best_word[0] == "WWW"
assert best_word[1] == 12

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_tie_prefers_ten_letters():
# Arrange
words = ["AAAAAAAAAA", "BBBBBB"]
Expand All @@ -62,6 +67,7 @@ def test_get_highest_word_tie_prefers_ten_letters():
assert best_word[0] == "AAAAAAAAAA"
assert best_word[1] == 18

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_tie_prefers_ten_letters_unsorted_list():
# Arrange
words = ["BBBBBB", "AAAAAAAAAA"]
Expand All @@ -73,6 +79,7 @@ def test_get_highest_word_tie_prefers_ten_letters_unsorted_list():
assert best_word[0] == "AAAAAAAAAA"
assert best_word[1] == 18

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_tie_same_length_prefers_first():
# Arrange
words = ["AAAAAAAAAA", "EEEEEEEEEE"]
Expand All @@ -86,6 +93,7 @@ def test_get_highest_word_tie_same_length_prefers_first():
assert best_word[0] == words[0]
assert best_word[1] == 18

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_many_ties_pick_first_ten_letters():
# Arrange
words = ["JQ", "FHQ", "AAAAAAAAAA", "BBBBBB", "TTTTTTTTTT"]
Expand All @@ -97,6 +105,7 @@ def test_get_highest_word_many_ties_pick_first_ten_letters():
assert best_word[0] == "AAAAAAAAAA"
assert best_word[1] == 18

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_many_ties_pick_shortest():
# Arrange
words = ["BBBBBB", "AAAAAAAAD", "JQ", "KFHK"]
Expand All @@ -108,6 +117,7 @@ def test_get_highest_word_many_ties_pick_shortest():
assert best_word[0] == "JQ"
assert best_word[1] == 18

#@pytest.mark.skip(reason="Temporarily disabled")
def test_get_highest_word_does_not_return_early_after_first_tiebreaker():
# Arrange
words = ["WWW", "MMMM", "BBBBBB", "AAAAAAAAD", "JQ", "KFHK"]
Expand Down