Skip to content

Snow Leopards - Anika and Aria#68

Open
ariastroud wants to merge 18 commits intoAda-C18:masterfrom
ariastroud:master
Open

Snow Leopards - Anika and Aria#68
ariastroud wants to merge 18 commits intoAda-C18:masterfrom
ariastroud:master

Conversation

@ariastroud
Copy link
Copy Markdown

No description provided.

Comment thread adagrams/game.py
for letter, frequency in LETTER_POOL_W_VALUES.items():
letters.append(letter)
frequency_of_letters.append(frequency['qty'])
# random.sample returns a new list containing elements from the population
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interesting approach! I appreciate that you added the comment about how the random.sample function works. It's a good idea, at this stage, to include comments like this when using functions from imported modules we haven't covered in the curriculum.

Expanding on that, it's wise to remember that not every language has the same built-in functions, so we should be able to implement this same functionality ourselves. In the projects, we don't ask for any features that we don't think you should be able to write yourself. For drawing a random hand, the only external function that would be "required" is a way to pick a random number (such as randint). At this stage in our coding journeys, it's often more valuable to re-implement things by hand to practice thinking about how to break down and approach the problem at hand, than to use a single library function to solve the problem for us.

Comment thread adagrams/game.py
@@ -1,11 +1,174 @@
import random

LETTER_POOL_W_VALUES = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an interesting approach to this data structure! A nested dictionary can sometimes be tricky to work with, especially if it's going to be transformed or looped over (with or without nesting) so we'll see how it fares in the code below!

Comment thread adagrams/game.py
def uses_available_letters(word, letter_bank):
pass
copy_letter_bank = letter_bank.copy()
for letter in word.upper():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works perfectly well! One thing to note is that you're looping through every letter in word on line 127 and then you're using the in operator to check if the letter is in the copy_letter_bank on line 128. Recall that using the in operator has O(n) time complexity, as does your for loop, so we've got O(n^2).

Rather than using a list of the letters in the hand, could we build a helper data structure (like a dictionary) that could let us look up the number of tiles remaining of each type?

If you have a frequency dict and then check if a key is in the dict, then you can leverage the fact that dicts constant time lookup (in contrast to the list's linear time look up).

Comment thread adagrams/game.py
pass
score = 0
for letter in word.upper():
for k, v in LETTER_POOL_W_VALUES.items():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This loop works for sure, but you could save the additional complexity of the inner for-loop and just use the dictionary itself, e.g.

        score += LETTER_POOL_W_VALUES[letter]["value"]

Comment thread adagrams/game.py
for k, v in LETTER_POOL_W_VALUES.items():
if k == letter:
score += v["value"]
if 6 < len(word) < 11:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread tests/test_wave_03.py

from adagrams.game import score_word

# @pytest.mark.skip
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No big deal, but since we didn't make any substantive changes to this file, it didn't need to be committed.

Comment thread adagrams/game.py
pass No newline at end of file
# list to store tuples
word_tuple_list = []
# make a tuple with word, score and length
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This approach does pass the test, but consider the tuple you're returning, which has three elements although the README instructions say to return a tuple with 2 elements (the word and the score). The tests pass because we're testing for the first and second elements of the tuple, e.g.

assert the_tuple[0] == "stuff"

But if we tested for the length of the tuple or for the explicit tuple, the test would fail, e.g.

    assert len(best_word) == 2

Comment thread adagrams/game.py

# element[1] is score in each tuple
# element[2] is len of each word in each tuple
for element in word_tuple_list:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this approach, it's a bit hard to mentally track the indices referred to, so it might be worth unpacking the elements into variables, in order to improve readability.

@marciaga
Copy link
Copy Markdown

marciaga commented Oct 4, 2022

Well done!

All your tests are passing and your code is laid out well overall. There's that one function I called attention to that works for the tests, but which would not work for other potential cases that it should. Check the comments for details. Nice use of using some built-ins as well your own approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants