C22 Phoenix Class - Luqi Xie

[shiqipaper]

No description provided.

[anselrognlie]

Looks good! Please review my comments, and let me know if you have any questions. Slack, our next 1:1, or office hours are preferred (rather than replying to my comments in GitHub) as I don't have email notifications turned on. Nice job!

[anselrognlie]

🎉 Nice job doing commits regularly as you finished each wave. A commit after each wave is a good target to aim for with projects. When working on your own code, it will be up to you to decide when it's a good time to commit, but the more we practice now, the more we'll be in the habit of committing. Otherwise, it's easy to forget.

[anselrognlie]

👍 I like that you represented the score chart with a global CONSTANT. Moving large data objects out of a function can improve the readability of the function by focusing on the logic.

However, we should be sure to store the data in a way that's convenient for our code to use. While the table in the README listed the scores as we see here, in practice, we need to look up the score for a letter, not which letters are associated with a score. With a dictionary, using the value we want to look up by as the key will lead to more efficient utilization of the dictionary. This would mean storing the scores more like

SCORE_CHART = {
    'A': 1,
    'B': 3,
    'C': 3,
    ...
}

[anselrognlie]

This code does a nice job of building up a list of all the available tiles. We could make this a little clearer by moving the logic to a helper function and giving it a good descriptive name, maybe build_tile_pile.

Having the variable here be letter_pool when we already have the global LETTER_POOL could be confusing. It's even a little more confusing in this case than in general because the variables are related. Consider a name such as tile_pile or maybe all_tiles.

[anselrognlie]

This line effectively has the meaning "run until we've picked 10 tiles." Some approaches to picking a tile aren't guaranteed to pick a tile each time through the loop, so phrasing the loop that way can be necessary. In your case, your loop block will always pick a tile each iteration, and so another way we think of this loop is, run the code 10 times. We could write that as

    for _ in range(10):

similarly to how you did the pile building a little earlier.

We could also consider giving a name to 10, like HAND_SIZE so that it's clear what this number represents here. For a small project like this, we probably know what all the numbers represent, but for larger systems, having unnamed numbers showing up all over the place makes the code more difficult to understand.

[anselrognlie]

👍 Using pop (rather than remove) lets us extract precisely the letter that was randomly picked. However, we'll see that popping from an aribtrary location has the same performance as using remove (it is related to the length of the list). But if we know exactly which position we want to pop, and the order of things in the list doesn't really matter, we can use a trick to pop from an aribitrary location in a way that doesn't depend on the length of the list. We can swap the value we want to pop to the end of the list, then pop from the end (popping from the end doesn't depend on the length of the list). Consider something like

        last_pos = len(letter_pool) - 1
        letter_pool[last_pos], letter_pool[letter_index] = letter_pool[letter_index], letter_pool[last_pos]
        letter_pool.pop()

[anselrognlie]

👍 A higher score word always replaces the best word so far.

[anselrognlie]

Since we're always replacing the best_word value (not changing a value within it) we could use a tuple throughout

            best_word = (word, score)  # parens are optional

But persoanlly, since we only ever update the score part here (where there's a new high score), but we replace the word in several palces, I prefer using two separate variaables until we reach the end, and only put them together at the return.

[anselrognlie]

Notice that both of these conditions check that the winning length isn't already 10 (another possible CONSTANT). A word can only beat another tied word if the word we already found wasn't length 10 already. So we could move the and len(best_word[0]) != 10 for both of these up to the elif score == highest_score line, basically encoding the check as "if the new word is tied and the old word can possibly be beaten".

But also, notice how complicated all of these checks are!

A different approach is to break up the parts of the logic into different chunks rather than trying to do it all in one loop. We can think of another approach like:

1. Find the max score (any winning word must have this score)
2. Make a list of all the words that have the max score
3. If we found only a single word, that's the winner
4. Otherwise, look at each word
5. If we find a 10 letter word, that's the winner (nothing would be able to beat it)
6. Otherwise, find the shortest word, that's the winner

The tie-breaking logic becomes a little easier thinking of the problem this way! Plus, each of those steps might be able to be written in their own helper function, making it easier to read the intention behind the logic, rather than needing to work backwards from the code to the intent, by which I mean it would be challenging to deduce what the rules to pick the winning word are solely from reading this code.

[anselrognlie]

Since this check doesn't do anything (it just returns to the top of the loop), but that's what would happen even without this condition, it's safe to remove this condition from the logic entirely.

[anselrognlie]

Nit: spacing around binary operator !=

            if len(word) == 10 and len(best_word[0]) != 10: