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

Sea Turtles - Celina B./Angelica R. #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wet-bulb
Copy link

@wet-bulb wet-bulb commented Apr 1, 2022

No description provided.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Nice job!

All waves are complete and all tests are passing! Your code is well organized, and you are generally using good, descriptive variable names. You used lists and dictionaries throughout, and were aware of when you needed to make copies to avoid side-effects.

The main thing I'd recommend thinking about, especially when working primarily with lists, is whether we can do a little pre-calculation here and there to build helper structures (often a frequency map of some kind stored in a dict) to improve the efficiency of our code. This is also true for data representation. Sometimes the most optimal way to store some data isn't the most optimal way to perform our needed lookups. And intermediate data transformation can give us the best of both worlds!

Overall, great work!

@@ -1,11 +1,113 @@
from os import truncate

Choose a reason for hiding this comment

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

Keep an eye out for unexpected imports. I don't see this being used, so it's likely VSCode "helpfully" pulled it in at some point.

from os import truncate
import random

LETTER_POOL = {

Choose a reason for hiding this comment

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

I also prefer pulling a big data structure like this out of any particular function into a module global. Putting this in the function directly would just make the function harder to read. We just need to be especially careful that we're not doing anything that would cause this to be modified, which would affect future uses of this data.

'Z': 1
}

LETTER_SCORES = {

Choose a reason for hiding this comment

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

While this representation of the scores is minimally repetitive, notice that when we look up scores in this structure later we're not able to use the optimal strategy of looking up directly by a piece of data we have (a letter) to get the value we don't (the score). Check my comments in score_word for some additional thoughts on this.

10: ["Q", "Z"]
}

def make_letter_pool_list():

Choose a reason for hiding this comment

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

👍 Nice helper to convert from the letter frequency data to a list where the letters present represent the actual "tiles".

letter_pool_list = []

for letter, count in LETTER_POOL.items():
letter_pool_list += [letter]*count

Choose a reason for hiding this comment

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

👍 Nice use of the the repeater operator!

This would also work as

        letter_pool_list += letter*count

The += operator more or less maps to list extend(), which takes an iterable, not necessarily another list. So the string repeater ("A"*3"AAA") itself produces an iterable (a string) where each character would be iterated over and then added to the pool list.

Comment on lines +93 to +96
score = score_word(word)
if score > highest_word[1]:
highest_word = (word, score)

Choose a reason for hiding this comment

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

When your implementation has distinct areas of code, it can be helpful to provide a comment on each section describing roughly what it does. This first part looks like it's locating the max score of any word (which word is incidental, but it will be whatever the first word in the list with this score was).

highest_word = (word, score)
for word in word_list:
score = score_word(word)

Choose a reason for hiding this comment

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

Here, we're double calculating the score for each word (we already did it once when looking for the max score). The scoring function isn't free, so we might build a helper structure once at the start of this function that we can use to store the score for each word once, then just look it up any other times we need it. What structure would let as store that information?

# to break the tie by returning the earlier word in the list
# only when we compare different words, hence the != conditional.
break
Copy link

@anselrognlie anselrognlie Apr 5, 2022

Choose a reason for hiding this comment

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

Breaking here would stop looking for any other shorter words. Maybe continue is what we want? It would skip over the current word, but would keep looking at later words in the list which could still be more favorable.

But if the only thing in the condition body is a continue do we even need this condition check?

It's a little difficult to get an intuitive sense of what this is doing, but a good takeaway from this is that if the comment explaining why this is like this is 4 times longer than the original line, maybe another pass is in order. 😄

break
elif len(word) == 10:
highest_word = (word, score)

Choose a reason for hiding this comment

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

I think it would be safe to immediately break after this. As soon as we find a word whose length is 10, nothing else can beat it.

Comment on lines +108 to +109
break

Choose a reason for hiding this comment

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

We may want to do this check earlier in the if/elif chain. These are checked in order, and once we find a 10 letter word, we don't need to do any more checks.

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.

2 participants