C19 Sapphire - Linh Huynh #116
Conversation
…y as individual elements within an empty list for easier selection later down the line.
… list under the variable hand
… a collection (or hand) of drawn letters.
…en a letter in the selected word is not present in the available collection (or hand) of letters.
…where user overuses a letter beyond the available amount.
…l points for inputted words. Sorry, I need to learn how to write more succinct git notes.
…s that appear more than once in a word.
…ut highest word score.
…y in a cleaner, more succinct manner by getting rid of unnecessary for loop iterations and eliminating the need of a SCORE_CHART_LOWERCASE dictionary.
… unsorted word lists.
…d with fewest letters in the event of a tie.
…tied score (for both scenarios where the winning word should have the fewest letters or 10 letters).
…the user hand from the letter pool.
There was a problem hiding this comment.
Great work on this project, Linh!
Overall, your project shows clear practice of iteration. I think it was really great how you thought about and used word_verification. However, I think there are places where the solution could be more efficient if you chose different ways to keep data. I'm seeing a lot of lists being used to hold a lot of things, when a boolean, another variable, or something else would do. I think it might be helpful to revisit this project and think about ways to simplify what you already have.
In addition, I'd like to encourage you to practice keeping your variables as local as possible, as a best practice around variable scope. Also, it makes for cleaner code style!
Lastly, I'd like to say that your git hygiene is great-- thank you for the messages that start with verbs and tell me clearly what the code change is. Please keep that up! :D
A test failure means that I'm marking this project as "yellow," as a sign to check-in about the feedback. This is my first time getting to know your code, and I know I'm a new reviewer. Please let me know what questions or comments you have, here or through Slack, any time!
| import random | ||
| from collections import Counter | ||
|
|
||
| LETTER_POOL = { |
| LETTER_POOL_LIST = [] | ||
| temp_list = [] | ||
|
|
||
| def draw_letters(): | ||
| pass |
There was a problem hiding this comment.
Your solution would benefit if we moved these two variables to inside the draw_letters function, so instead:
def draw_letters():
LETTER_POOL_LIST = []
temp_list = []
# ...The first sign that we should move these variables is because they are only used/modified/accessed within one function: the draw_letters function. We want our variables to have the smallest scope as possible-- so if other functions don't need access to LETTER_POOL_LIST and temp_list, then let's make them local variables.
As a note, if we move these variables, all of your tests pass! Please let me know if you'd like to talk together about why this fix works.
| for sublist in temp_list: | ||
| for letter in sublist: |
There was a problem hiding this comment.
For me, it's hard to understand what sublist represents by its name. I would suggest renaming sublist to letter_list or something else more specific, and if you're having trouble coming up with a name, to revisit what it's used for
|
|
||
| return hand | ||
|
|
||
| draw_letters() |
There was a problem hiding this comment.
This line doesn't do anything meaningful right now, since it's not in a test file or a function! This line is safe to delete.
| for letter in letter_bank: | ||
| if letter not in hand_letter_count: | ||
| count = 1 | ||
| hand_letter_count[letter] = count | ||
| else: | ||
| count += 1 | ||
| hand_letter_count[letter] = count |
There was a problem hiding this comment.
Your logic is sound here, but there are a couple of ways we can make it clearer by focusing on count. Firstly, count is only used in one place: inside this single for loop. Therefore, we could move the definition of count:
for letter in letter_bank:
count = 0
if letter not in hand_letter_count:
count = 1
hand_letter_count[letter] = count
else:
count += 1
hand_letter_count[letter] = countAfterwards, we can really see how count is being used. We could even find a way to refactor it out and avoid it:
for letter in letter_bank:
if letter not in hand_letter_count:
hand_letter_count[letter] = 1
else:
hand_letter_count[letter] += 1This code has the exact same logic as your original project, and three less lines and one less variable!
| if word == "": | ||
| word_score["empty"] = 0 |
There was a problem hiding this comment.
This code says to me "if word is empty, then the total score is 0". I think that adding "empty" into word_score isn't as straightforward as we can be, although I see that you're trying to give the score 0 even if the word has a length of 0 (an empty string). An alternative solution is to return 0 inside this conditional:
if word == "":
return 0| word_score = {} | ||
|
|
||
| if word == "": | ||
| word_score["empty"] = 0 | ||
|
|
||
| for letter in word.upper(): | ||
| if letter in SCORE_CHART: | ||
| if letter not in word_score: | ||
| word_score[letter] = SCORE_CHART[letter] | ||
| else: | ||
| word_score[letter] += SCORE_CHART[letter] | ||
|
|
||
| if 7 <= len(word) <= 10: | ||
| word_score["bonus"] = 8 | ||
|
|
||
| return sum(word_score.values()) |
There was a problem hiding this comment.
I think your approach to this problem is interesting and extensible-- I think that adding "bonus" key and summing word_score.values() is really smart for an expanded project. In this situation, if you wanted to refactor and make word_score an integer, instead of a {}, then that would also pass the tests.
|
|
||
| for word, score in score_dict.items(): | ||
| if score in matching_scores: | ||
| matching_scores_dict[word] = score |
There was a problem hiding this comment.
From what I can tell, this code is finding all times that there is a non-unique score, and collects them in a dictionary between word and score. However, do we need to keep a dictionary of matching scores to find the best word?
| matching_scores_dict[word] = score | ||
|
|
||
| for word, score in matching_scores_dict.items(): |
There was a problem hiding this comment.
Instead of iterating through matching_scores.dict.items(), what if we iterate through score_dict instead? score_dict is a dictionary that already keeps each word and its score. We already have a value for highest_score and winning_word, too!
If we refactor this loop so it iterates through score_dict, we can also delete the code around matching scores. If we make these changes, then the tests still pass!
| @@ -38,6 +38,7 @@ def test_draw_letters_draws_ten(): | |||
| # Assert | |||
| assert len(letters) == 10 | |||
|
|
|||
| #@pytest.mark.skip(reason="no way of currently testing this") | |||
There was a problem hiding this comment.
Would be good to delete these lines in a commit
|
Hi Simon,
It was nice meeting you last week. Thank you so much for detailed feedback
and grading my Adagrams submission. I found your comments very helpful and
appreciate the time you invested in writing them.
The following is not an excuse by any means, however I was sick during the
week of the Adagrams submission deadline and ended up taking an excused
absence on Thursday. Due to my limitations in time, health, and energy as a
result, I wasn't able to dedicate as much time as desired to checking and
refactoring my code prior to submission. As you can see, there were
accidental oversights like forgetting to remove line 87 where I called the
draw_letters() function as a means of testing it (when I previously had
print statements included in the function). I thought I had already removed
that line along with all my print statements. Chief amongst my silly
mistakes was embarrassingly misplacing my LETTER_POOL_LIST and temp_list
variables in the global environment rather than locally within the
draw_letter() function, which embarrassingly yielded 1 test failure that
otherwise would've passed if I caught my error. This is my fault for
rushing with the limited time that I had on my hands. At that point, I
figured it was more important to prioritize code functionality and submit
my completed code on time.
Would it be possible to correct, refactor my code, and resubmit? I'm not
sure if this is allowed at Ada and understand if it isn't.
Since I am slowly recovering from a pesky flu and luckily felt better last
week, I was able to ensure better time management and alloted ample time to
complete the Viewing Party project with 1 day to spare for refactoring or
polishing the code prior to submission. Hopefully, it'll allow for better
readability, ease, and grading. :)
Kind regards,
Linh Huynh
…On Thu, Mar 30, 2023 at 3:38 PM simon ***@***.***> wrote:
***@***.**** commented on this pull request.
Great work on this project, Linh!
Overall, your project shows clear practice of iteration. I think it was
really great how you thought about and used word_verification. However, I
think there are places where the solution could be more efficient if you
chose different ways to keep data. I'm seeing a lot of lists being used to
hold a lot of things, when a boolean, another variable, or something else
would do. I think it might be helpful to revisit this project and think
about ways to simplify what you already have.
In addition, I'd like to encourage you to practice keeping your variables
as local as possible, as a best practice around variable scope. Also, it
makes for cleaner code style!
Lastly, I'd like to say that your git hygiene is great-- thank you for the
messages that start with verbs and tell me clearly what the code change is.
Please keep that up! :D
A test failure means that I'm marking this project as "yellow," as a sign
to check-in about the feedback. This is my first time getting to know your
code, and I know I'm a new reviewer. Please let me know what questions or
comments you have, here or through Slack, any time!
------------------------------
In adagrams/game.py
<#116 (comment)>:
> @@ -1,11 +1,170 @@
+import random
+from collections import Counter
+
+LETTER_POOL = {
Great call making these constants
------------------------------
In adagrams/game.py
<#116 (comment)>:
> +LETTER_POOL_LIST = []
+temp_list = []
+
def draw_letters():
- pass
Your solution would benefit if we moved these two variables to inside the
draw_letters function, so instead:
def draw_letters():
LETTER_POOL_LIST = []
temp_list = []
# ...
The first sign that we should move these variables is because they are
only used/modified/accessed within one function: the draw_letters
function. We want our variables to have the smallest scope as possible-- so
if other functions don't need access to LETTER_POOL_LIST and temp_list,
then let's make them local variables.
As a note, if we move these variables, all of your tests pass! Please let
me know if you'd like to talk together about why this fix works.
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + for sublist in temp_list:
+ for letter in sublist:
For me, it's hard to understand what sublist represents by its name. I
would suggest renaming sublist to letter_list or something else more
specific, and if you're having trouble coming up with a name, to revisit
what it's used for
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + for letter in sublist:
+ LETTER_POOL_LIST.append(letter)
+
+ # Chooses 10 random letters as hand, excluding the ones that are already chosen.
+ #hand = random.sample(LETTER_POOL_LIST, 10)
+
+ hand = []
+ while len(hand) < 10:
+ random_letter = random.choice(LETTER_POOL_LIST)
+ random_letter_index = LETTER_POOL_LIST.index(random_letter)
+ LETTER_POOL_LIST.pop(random_letter_index)
+ hand.append(random_letter)
+
+ return hand
+
+draw_letters()
This line doesn't do anything meaningful right now, since it's not in a
test file or a function! This line is safe to delete.
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + for letter in letter_bank:
+ if letter not in hand_letter_count:
+ count = 1
+ hand_letter_count[letter] = count
+ else:
+ count += 1
+ hand_letter_count[letter] = count
Your logic is sound here, but there are a couple of ways we can make it
clearer by focusing on count. Firstly, count is only used in one place:
inside this single for loop. Therefore, we could move the definition of
count:
for letter in letter_bank:
count = 0
if letter not in hand_letter_count:
count = 1
hand_letter_count[letter] = count
else:
count += 1
hand_letter_count[letter] = count
Afterwards, we can really see how count is being used. We could even find
a way to refactor it out and avoid it:
for letter in letter_bank:
if letter not in hand_letter_count:
hand_letter_count[letter] = 1
else:
hand_letter_count[letter] += 1
This code has the exact same logic as your original project, and three
less lines and one less variable!
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + for letter in capitalized_word:
+ letter_quantity = capitalized_word.count(letter)
Reinforcing a comment I've made, word_letter_count is only used in one
place: within this for loop. We can move the definition of
word_letter_count from above, closer to where it's being used.
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + word_verification.append("True")
+ else:
+ word_verification.append("False")
word_verification is a list, and lists can contain Booleans. With the way
that you're using word_verification, I think you could use the booleans
True and False instead of the strings "True" and "False"
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + if "False" in word_verification:
+ return False
+ else:
+ return True
This logic says "If we've come across even one False is inside
word_verification, then we'll return False." Using the same logic, could
we refactor this if conditional? One thing to notice is that we're using
False/"False" and word_verification in two of the same places. Could we return
False in the case that we find the False case?
We can move return False, and assume that we return True otherwise.
I'd encourage you to work through this refactoring and see if any
questions come up. After refactoring that, I think it's worth asking "Do I
need the word_verification variable? Can I get rid of it? Why/why not?"
------------------------------
In adagrams/game.py
<#116 (comment)>:
>
def score_word(word):
- pass
+ word_score = {}
+
+ if word == "":
+ word_score["empty"] = 0
[nit] Your indentation on this line is off by one space
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + if word == "":
+ word_score["empty"] = 0
This code says to me "if word is empty, then the total score is 0". I
think that adding "empty" into word_score isn't as straightforward as we
can be, although I see that you're trying to give the score 0 even if the
word has a length of 0 (an empty string). An alternative solution is to return
0 inside this conditional:
if word == "":
return 0
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + word_score = {}
+
+ if word == "":
+ word_score["empty"] = 0
+
+ for letter in word.upper():
+ if letter in SCORE_CHART:
+ if letter not in word_score:
+ word_score[letter] = SCORE_CHART[letter]
+ else:
+ word_score[letter] += SCORE_CHART[letter]
+
+ if 7 <= len(word) <= 10:
+ word_score["bonus"] = 8
+
+ return sum(word_score.values())
I think your approach to this problem is interesting and extensible-- I
think that adding "bonus" key and summing word_score.values() is really
smart for an expanded project. In this situation, if you wanted to refactor
and make word_score an integer, instead of a {}, then that would also
pass the tests.
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + matching_scores = [k for k,v in Counter(score_list).items() if v>1]
+
+ for word, score in score_dict.items():
+ if score in matching_scores:
+ matching_scores_dict[word] = score
From what I can tell, this code is finding all times that there is a
non-unique score, and collects them in a dictionary between word and score.
However, do we need to keep a dictionary of matching scores to find the
best word?
------------------------------
In adagrams/game.py
<#116 (comment)>:
> + score = score_word(word)
+ score_dict[word] = score
+
+ for word, score in score_dict.items():
+ score_list.append(score)
+ if score > highest_score:
+ highest_score = score
+ winning_word = word
+
+ matching_scores = [k for k,v in Counter(score_list).items() if v>1]
+
+ for word, score in score_dict.items():
+ if score in matching_scores:
+ matching_scores_dict[word] = score
+
+ for word, score in matching_scores_dict.items():
Instead of iterating through matching_scores.dict.items(), what if we
iterate through score_dict instead? score_dict is a dictionary that
already keeps each word and its score. We already have a value for
highest_score and winning_word, too!
If we refactor this loop so it iterates through score_dict, we can also
delete the code around matching scores. If we make these changes, then the
tests still pass!
------------------------------
In tests/test_wave_01.py
<#116 (comment)>:
> @@ -38,6 +38,7 @@ def test_draw_letters_draws_ten():
# Assert
assert len(letters) == 10
***@***.***(reason="no way of currently testing this")
Would be good to delete these lines in a commit
—
Reply to this email directly, view it on GitHub
<#116 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2YFILQHSTAPWAS56H5CPSLW6YDO7ANCNFSM6AAAAAAWGYUHSM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
No description provided.