C22 Tami Gaertner #37
Conversation
| @@ -0,0 +1,28 @@ | |||
| score_of_letters = { | |||
There was a problem hiding this comment.
Since we're treating this data structure like a constant, we should follow constant variable naming conventions and use all capital letters like: SCORE_OF_LETTERS = ...
| @@ -0,0 +1,28 @@ | |||
| LETTER_POOL = { | |||
There was a problem hiding this comment.
I love the thought to separate out data from our implementation code. In larger projects, especially when the data is shared by multiple functions or files, this makes a lot of sense.
There is a little bit of a tradeoff happening when only one function uses the data. By placing the data in a larger scope, more code has access to and could potentially change the values in the data structure. If only one function will ever use the data, it can make sense to place the data inside the function. That way the data is only created when the function is called, and only the code that need access to the data has the ability to see or change it.
| if not available_letters: | ||
| break |
There was a problem hiding this comment.
Will we ever reach a situation where if not available_letters will evaluate to True?
adagrams/game.py
Outdated
| letter_counts = LETTER_POOL.copy() | ||
|
|
||
| while len(hand)< 10: | ||
| # create a list of availbe letters | ||
| available_letters =[] | ||
| # check each letter and add ot availble_letters if greater than 0 | ||
| for letter, count in letter_counts.items(): | ||
| if count > 0: | ||
| available_letters.append(letter) |
There was a problem hiding this comment.
This implementation creates 3 main data structures, our hand we're filling, letter_counts to track used tiles, and available_letters which is recreated each iteration of the while loop. Another approach could be to create available_letters once outside of the while loop, then inside the loop, remove a letter from available_letters after it is selected. How would that impact the implementation and how many data structures we need to keep track of our hand and used tiles?
adagrams/game.py
Outdated
| hand.append(choose_letter) | ||
|
|
||
| # convert letter to str | ||
| return ''.join(hand) |
There was a problem hiding this comment.
I would like to see this return value updated. One of the requirements for this function was that it returns an array of ten strings, so we want to leave hand as an array rather than convert the contents to a string.
The tests all pass because none of them are directly asserting that the return value is of type list. Both lists and strings are iterable (able to be iterated over) in Python, so when the tests try to iterate over the return value to check that each index is a single letter or that we haven't selected a certain letter too many times, they don't see anything wrong.
| # add to get total score | ||
| total_score += letter_score | ||
| # bonus, if there are 7 or more letters in the word then add 8 points to the total | ||
| if len(word) >= 7 : |
There was a problem hiding this comment.
| if len(word) >= 7 : | |
| if len(word) >= 7: |
| # loop over the letters in the word | ||
| for letter in word: | ||
| # find the letter in the score_of_letters dict | ||
| if letter.upper() in score_of_letters: |
There was a problem hiding this comment.
Nice check to let us keep scoring if we have something like a hyphenated word!
| # initialize tuple, 1st stores the word, 2nd store the score | ||
| highest_score = (None, 0) |
There was a problem hiding this comment.
Great idea to use a tuple from the start so you have your return value ready to go by the end of the loop!
adagrams/game.py
Outdated
| highest_score = (word, new_score) | ||
| # if no word is 10 letters long, and if the lenth of the word is shorter than high_score word chose the shorter word | ||
| elif len(highest_score[0]) != 10 and len(word) < len(highest_score[0]): | ||
| highest_score = (word, new_score) |
There was a problem hiding this comment.
Both sides of the if/elif have the same contents highest_score = (word, new_score). 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_score[0]) != 10
is_new_word_smaller = len(highest_score[0]) != 10 and len(word) < len(highest_score[0])
if is_new_word_ten or is_new_word_smaller:
highest_score = (word, new_score)| highest_score = (None, 0) | ||
|
|
||
| for word in word_list: |
There was a problem hiding this comment.
Great approach to tie break in a single loop over word_list!
No description provided.