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.

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

AC2 Ocelots Class - Catherine Bandarchuk #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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
117 changes: 113 additions & 4 deletions adagrams/game.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,120 @@
import random

# Dictionary of letters and available number of duplicates of each letter
POOL_LETTERS_DICT = {
'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 }
Comment on lines +4 to +7

Choose a reason for hiding this comment

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

These lines run longer than the PEP8 style guide recommends (79 characters). Even though it will take considerably more space in the file, I would suggest structuring this dictionary with one key/value pair on each line to make it as easy as possible for readers to quickly see and understand the contents.


# Dictionary of letters and points it give to the user. keys: points, values: lists of letters
SCORE_CHART = {
1: ['A', 'E', 'I', 'O', 'U', 'L', 'N', 'R', 'S', 'T'],
2: ['D', 'G'],
3: ['B', 'C', 'M', 'P'],
4: ['F', 'H', 'V', 'W', 'Y'],
5: ['K'],
8: ['J', 'X'],
10: ['Q', 'Z']
}


def create_pool_letters_list(POOL_LETTERS_DICT):

Choose a reason for hiding this comment

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

I would recommend choosing a parameter name that is different from the constant data structure POOL_LETTERS_DICT to help avoid confusion. Since this is a parameter, it's important to remember anything could be passed in as an argument, it isn't guaranteed to be the dictionary POOL_LETTERS_DICT. Style-wise, in Python we usually stick to lowercase variable names unless they are a global constant, so I would recommend a lowercased name for the parameter.

'''
This function convert POOL_LETTERS_DICT into the available_letters_list;
has parameter = POOL_LETTERS_DICT;
Returns available_letters_list which contain letters duplicates as shown in value of the key(letter) in dictionary
'''
available_letters_list = list()
for letter in POOL_LETTERS_DICT:
for i in range(POOL_LETTERS_DICT[letter]):
available_letters_list.append(letter)
Comment on lines +27 to +30

Choose a reason for hiding this comment

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

They all perform similar actions under the hood, but we could combine lines 29 & 30 in a couple different ways with list's extend method:

for letter in POOL_LETTERS_DICT:
    available_letters_list.extend(letter * POOL_LETTERS_DICT[letter])

# Another option using extend with a list comprehension
for letter in POOL_LETTERS_DICT:
    available_letters_list.extend(letter for i in range(POOL_LETTERS_DICT[letter]))

More info on list comprehensions if you're interested: https://www.w3schools.com/python/python_lists_comprehension.asp

return available_letters_list
Comment on lines +27 to +31

Choose a reason for hiding this comment

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

Nice helper function! Great way to make sure the original POOL_LETTERS_DICT doesn't change between calls to draw_letters.


def draw_letters():
pass
'''
This function has no parameters;
Returns an array of ten strings (each one letter which randomly drawn from available_letters_list
'''
number_of_letters = 10
all_letter_list = create_pool_letters_list(POOL_LETTERS_DICT)

Choose a reason for hiding this comment

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

It looks like POOL_LETTERS_DICT is only accessed by the draw_letters function. If we didn't want to manage worrying about changes to POOL_LETTERS_DICT between calls to draw_letters, another option could be to declare the data structure inside the draw_letters function. There are tradeoffs, the structure would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.

player_letters_list = ['','','','','','','','','','']
for i in range(number_of_letters):
random_index = random.randint(0, len(all_letter_list) - 1)
player_letters_list[i]= all_letter_list.pop(random_index)
Comment on lines +40 to +43

Choose a reason for hiding this comment

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

This is a solid implementation! If we were in a system where memory was limited and didn't want to start player_letters_list with empty strings we know we will overwrite, another option could be to start with an empty list and use a while loop that stops when the list has 10 elements.

player_letters_list = []
while len(player_letters_list) != 10:
    random_index = random.randint(0, len(all_letter_list) - 1)
    new_letter = all_letter_list.pop(random_index)
    player_letters_list.append(new_letter)

return player_letters_list

def uses_available_letters(word, letter_bank):
pass
'''
This function has two parameters: input word (string) letter_bank (array of drawn letters from draw_letters function)
Returns True if every letter in the input word is in the letter_bank
Returns False if there is a letter in input that is not present in the letter_bank or has too much of compared to the letter_bank
'''
checked_word = list()
word_ls = []
Comment on lines +52 to +53

Choose a reason for hiding this comment

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

I see a combination of how lists are declared across the file. One of the most important things for writing easy to read and understand code is consistency through a project. I recommend trying out using brackets to declare empty lists or lists with contents, and using list() only when we need the function for actions like casting another data type to a list.

word_ls[:] = word.upper()
for letter in word_ls:
Comment on lines +53 to +55

Choose a reason for hiding this comment

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

One of the cool things about strings in python is that they are iterable - we can loop through them like a list without needing to explicitly convert them to a list first:

word_ls = word.upper()
for letter in word_ls:
...

if letter in letter_bank:
checked_word.append(letter)
index = letter_bank.index(letter)
del letter_bank[index]

Choose a reason for hiding this comment

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

This is a solid approach, but what if it was important that we did not alter our incoming parameters? How might that change the implementation?

if checked_word == word_ls:
letter_bank.extend(checked_word)
letter_bank.sort()
return True
return False

Choose a reason for hiding this comment

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

Something to consider: this approach will go through the whole word before it returns False. However, the moment we hit any letter that isn't in the word bank, we know the word is invalid. How could we save some processing time by exiting early when we find an invalid letter?


def score_word(word):
pass
'''
This function has one parameter: word, which is a string of characters
Returns an integer representing the number of points
Each letter within word has a point value (located in SCORE_CHART dictionary)
If the length of the word is 7, 8, 9, or 10, then the word gets an additional 8 points
'''
user_points = 0
word_ls = []
word_ls[:] = word.upper()
if len(word_ls)>= 7:
user_points = 8
for element in SCORE_CHART:
for letter in word_ls:
if letter in SCORE_CHART[element]:
user_points += element
Comment on lines +78 to +81

Choose a reason for hiding this comment

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

Worst-case scenario this approach will loop through all values in SCORE_CHART for each letter in the input word for every key in SCORE_CHART. If it was important to reduce processing time, we might want to consider other options for how we structure the scoring data. One option could be to keep the scores in a dictionary similar to POOL_LETTERS_DICT where the keys are letters and the values are their scores. This would let us loop over the characters in word, and skip any letters that aren't in SCORE_CHART.

user_points = 0
for letter in word:
    if letter.upper() in SCORE_CHART:
        user_points += SCORE_CHART[letter]

# another option using dictionary's `get()` function
user_points = 0
for letter in word:
    # Here `get` will return the value for the key `letter.upper()` or 0 if that key does not exist
    user_points += SCORE_CHART.get(letter.upper(), 0)

return user_points

def get_highest_word_score(word_list):
pass
'''
This function has one parameter - word_list( list of strings)
Returns a tuple that represents the data of a winning word and it's score.
In the case of tie in scores:
1 - the word which has 10 letters
2 - prefer the word with the fewest letters
3 - the first one in the supplied list
'''
users_score = 0

Choose a reason for hiding this comment

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

Since the value of users_score is overwritten at the start of each loop iteration and does not get used outside of the for-loop, I would remove the declaration here and let it be instantiated for the first time inside the for-loop on line 97 where we call score_word.

max_score = 0
max_word = ''
for word in word_list:
users_score = score_word(word)
if users_score > max_score:
max_score = users_score
max_word = word
elif users_score == max_score:
if len(word) < len(max_word):
if len(max_word) == 10:
continue
else:
max_score = users_score
max_word = word
elif len(word) == len(max_word):
word_index = word_list.index(word)
max_word_index = word_list.index(max_word)
if word_index > max_word_index:
continue
else:
max_word = word
max_score = users_score
Comment on lines +109 to +115

Choose a reason for hiding this comment

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

A for-in loop will iterate through a list from index 0 until the end of the list (in order), which means that word_index will always be greater than max_word_index. If we know that 2 words have the same score and the same length, do we ever need to update variables in lines 109-115? What happens if we remove that code?

elif len(word) == 10:
max_word = word
max_score = users_score
Comment on lines +96 to +118

Choose a reason for hiding this comment

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

Really nice approach, I love the searching and tie-breaking in a single pass over the input word_list !

best_word = (max_word, max_score)
return best_word # tuple of winning word and it's scores best_word[ 0 - word , 1 - it's score]
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ packaging==20.8
pluggy==0.13.1
py==1.10.0
pyparsing==2.4.7
pytest==6.2.1
pytest==6.2.5
toml==0.10.2