Dragonfly class - Lina Martinez

[linakl19]

No description provided.

[yangashley]

Nice job on Adagrams! Good work making frequent commits :)

[yangashley]

👍 Nice job using all capital letters to name this constant variable!

[yangashley]

👍 Since we declare i but don't access it, prefer to use _ instead, like:

Suggested change

	for letter, quantity in LETTER_POOL.items():
	total_letter_pool.extend([letter for i in range(quantity)])
	for letter, quantity in LETTER_POOL.items():
	total_letter_pool.extend([letter for _ in range(quantity)])

[yangashley]

Prefer to have this logic for creating a total letter pool in a helper function like create_letter_pool. Then you can call that helper function in draw_letters which would make it more concise and more single responsibility (instead of needing to create a pool and draw letters)

[yangashley]

If I did not know how to play Adagrams, I would not understand the significance of 10. If I were a dev who tried to read this, I might be wondering "Why do we only loop 10 times"?

It would be more descriptive to use a constant variable to reference 10 so that your code is more self-documenting.

NUM_TILES_ALLOWED_IN_HAND = 10
for _ in range (NUM_TILES_ALLOWED_IN_HAND):

[yangashley]

Suggested change

	random_index = randint(0,len(total_letter_pool)-1)
	random_index = randint(0, len(total_letter_pool) - 1)

You're missing white spaces after the comma and we should surround operators like - with white spaces. It's a small thing, but as you write more code (and eventually contribute to a large codebase) it's important to be consistent with whitespaces so that the whole project stays neat.

You can review the PEP8 style guide on white spaces here.

[yangashley]

If I want to pop something by index, I find the element by index (which is pretty efficient) and then remove it. After I remove the element, I've left an empty space in my list and I have to move the other elements to fill that empty space, this is not an efficient operation because how much shifting I need to do to fill the empty space depends on the size of my list (the bigger the list, the more shifting).

We'll hear more about this as we work on Big O material, but we often want to minimize performance costs in code.

However, if you pop from the end of a list then we don't need to do any shifting in the list because you don't leave an empty space somewhere in the middle. 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 a random 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, like:

last_pos = len(total_letter_pool) - 1 
# this is the syntax for swapping elements in a list
# for example linas_list[0], linas_list[3] = linas_list[3], linas_list[0] swaps the elements at 0 and 3 position
total_letter_pool[last_pos], total_letter_pool[random_index] = total_letter_pool[random_index], letter_pool[last_pos] 
letter_pool.pop()

[yangashley]

Nice! I like that you check if the letter doesn't exist and immediately return False. This allows the 'main' logic on line 65 to be unindented and we don't need additional conditional logic.

[yangashley]

The performance of both checking whether something is in a list, and the remove function depends on the length of the list we are looking at. If I want to find something in a list, I have to visit each element until I find it (line 63). If I want to remove something, I have to visit each element, find it and then remove it (line 65). After I remove the element, I've left an empty space in my list and I have to move the other elements to fill that empty space.

And since we are doing these operations in a loop, it compounds the cost.

To improve the performance, we could iterate over letter_bank once to build a dictionary holding a count of how many times each letter appears. As we process each letter in the word, we still need to look it up in the dictionary and check the count, but these are more efficient in a dictionary than in a list. Then we can just update the count by -= 1 instead of removing an element.

[yangashley]

If I had no idea about how to play Adagrams, I'm left wondering "Why do we add 8 to the total_points when the length of the word is greater than 7"

We can make our code more self-documenting by introducing a constant variable, like so:

BONUS_POINTS_FOR_LENGTH = 8
    if len(input_word) >= 7:
        total_score += BONUS_POINTS_FOR_LENGTH

[yangashley]

Consider putting the tie-breaking logic inside a helper function like break_tie to make get_highest_word_score more concise and single-responsibility.

[yangashley]

Notice that you create winner on line 110, you don't do anything with the variable and then you immediately return it on line 111.

Since we don't actually use the variable, so we could delete line 110 and then just return the tuple without naming it, like this:

Suggested change

	winner = (winner_word, winner_score)
	return winner
	return winner_word, winner_score

[yangashley]

POINTS is a dictionary where the values are lists.

POINTS = {
    1:["A", "E", "I", "O", "U", "L", "N", "R", "S", "T"],
    # etc, etc

This means for every char in input_word, I have to check if that char is in the list letter_score. Even though you didn't write a loop on line 75 if char in letter_score, using the in operator with a list means that Python has to check every letter in the letter_score list to find char.

We'll discuss more in Big O about the finer details, but what I'm getting at is that because POINTS has inner lists, we decrease the performance. Instead, I'd prefer that POINTS was a dictionary with 26 k/v pairs because looking up values in a dictionary is faster than a list.

For example:

POINTS = {
    A: 1, 
    B: 3, 
    # etc, etc
}