Bumblebee - Rachana P #22

rachanapatel · 2025-03-20T22:15:52Z

No description provided.

anselrognlie

You've got a lot of good ideas here! Please review my comments, and let me know if you have any questions.

anselrognlie · 2025-03-25T06:41:28Z

adagrams/game.py

@@ -1,13 +1,127 @@
 from random import randint

-def draw_letters():
-    pass
+letter_dist: list[dict] = [{"A":9}, {"B":2}, {"C": 2}, {"D": 4}, {"E": 12}, {"F": 2}, 


I'm curious what prompted you to include type hints. Have you run across them in your own studies? We will not be covering type hints in the curriculum aside from a brief tangent in Unit 2, so while you may feel free to practice using them in individual work, please do not use type hints in any group work.

But in general, I'd recommend setting type hints aside for the time being. The curriculum work was not created with type hinting in mind, and so you may find yourself fighting with the type hinting to get things to work properly more than working on the actual project materials.

For Python in particular, type hints are somewhat problematic. They can help our editors provide better suggestions, but Python essentially ignores the type hints when the code is actually running, so it doesn't natively provide any additional safety. To get the most out of type hints, we need to run external programs, such as mypy to confirm that we are following the type hints properly. To make things more confusing, different type checking utilities can report different issues with our type hints.

If we have a variable that shouldn't change, especially one that's global, prefer to use CONSTANT_CASE.

I see that you're using a list as the main structure so that you are able to access a random entry by its index, since we are only picking random numbers. Each entry is serving to associate a letter (tile) with its available count, and there is only one letter-count pair in each position.

As a result, consider using a nested tuple or list rather than a nested dict. If we don't know the keys in a dictionary, it can be cumbersome to retrieve the values from it, but if we use a pair, then we would know that the letter is position 0 and the count is in position 1. Using dictionaries when there is only a single pair in each is less well-suited to this data arrangement.

LETTER_COUNTS = [("A", 9), ("B", 2}, ("C", 2), ..., ("Z", 1)]

Yup, came across type hinting online and thought it could be useful for a reader but I see what you mean

anselrognlie · 2025-03-25T06:58:24Z

adagrams/game.py

+    generated_dict = {}
+    #use randint to access a random dictionary in list letter_dist
+    while len(generated_dict.keys()) < 10:


👀 This will loop until 10 disctinct letters have been chosen, no matter how many times each letter was chosen, resulting in a hand of 10 distinct letters rather than 10 letters, some of which may be duplicated.

Note that it's unnecessary to do len of the keys collection. We will get the same result as getting the len of the entire directory.

Though the key issue here is that using the length of the dictionary will result in the tile distribution issue mentioned above, so I'm just mentioning this as a general point, not to imply that changing how the length of the dictionary is being determined is the issue at hand.

Changed the while loop to rely on a counter that increases when a valid letter is selected and added to the dictionary, rather than the length of the dictionary

anselrognlie · 2025-03-25T07:02:51Z

adagrams/game.py

The for loop and the use of items here is a direct result of storing the letter and count information in a dict. Even though we know there's only one pair in each dict, we don't have a ready way of getting that specific pair. If we instead stored tuples with the letter and count information as described above, these lines could become

char, max_allowed = letter_dist[randint(0, len(letter_dist)-1)]

anselrognlie · 2025-03-25T07:05:31Z

adagrams/game.py

+                random_letter = char
+                generated_dict[random_letter] = 1
+            else:
+                if max_allowed > generated_dict[random_letter]:
+                    generated_dict[random_letter] += 1


Using a dictionary with a value that can track the used tile count is a great idea! But what if we also kept a count of the total tiles so that we knew when we had picked 10 total tiles, as well as how many of each tile had been chosen? Could we use that information to stop looping once we had 10 total tiles?

anselrognlie · 2025-03-25T07:08:23Z

adagrams/game.py

+                    generated_dict[random_letter] += 1
+
+    return generated_dict.keys()


As written, returning the key will end up returning 10 unique tiles (preventing us from getting, say, 3 Es, which should be possible). Could we process the dictionary count information to produce a list of the picked tiiles with the proper number of each that were picked?

anselrognlie · 2025-03-25T14:40:45Z

adagrams/game.py

+                shortest_len_word = tied_words
+                winner = shortest_len_word
+                break


👀 This condition is close to what we need, but it could lead to the selection of the incorrect word. The shortest word could appear anywhere in the list, and there could be any number of word lengths from 9 (10 would land in the previous condition) on down. This means that we might need to update the shortest word variable multiple times as we move through the list, but this logic will allow the shortest word to be updated only once before it assumes it has found the winner.

For example, given the "words" "AAAA", "DD", and "F" (all are score 4), the code would find "DD" to be the winner rather than "F" which is shorter.

anselrognlie · 2025-03-25T14:45:46Z

adagrams/game.py

+    if len(ties) > 0:
+        winning_word = tiebreaker(ties)
+        winning_score = score_tracker[winning_word]


👀 If we're in a tiebreaking condition, winning_score should already have been set during the initial tie detection, meaning we shouldn't need to refresh the value here. If this was found to be necessary, then there could be a disconnect between determining the case of a lone winning word versus a tie.

Is it possible that the tiebreaking code code be running even when a singular winner comes along after the tie has been detected , meaning we no longer need to break the tie?

anselrognlie · 2025-03-25T14:47:47Z

adagrams/game.py

+            winning_score = points
+            winning_word = word_item


👀 When we find a word with a new best score, this will still leave the ties list intact. If a better score comes along, then that means anything that we previous thought was a tie is no longer a tie. How can we stop tracking those words that were previously considered to be ties?

We can clear out the ties list

anselrognlie · 2025-03-25T14:49:47Z

adagrams/game.py

+
+def get_highest_word_score(word_list: list) -> tuple[str, int]:
+    score_tracker: dict[str, int] = {} 


The code does use the score_tracker to look up a score using the word as a key later on, but I don't think that should be necessary. Instead, we have a data structure that is primarily being used to iterate through some pairs, so this could be a list of nested tuples or lists rather than a dictionary. Prefer to use dictionaries when key-value access is the primary use of the data structure.

anselrognlie · 2025-03-25T15:38:54Z

adagrams/game.py

+
+# get highest value or add values to ties list
+    for word_item, points in score_tracker.items():


👍Great idea to have this logic focus on looking for scores and leaving the tiebreaking until later.

There are still some tricky points that we need to consider here, especially around building up (and potentially clearing) our list of ties as we continue to find new scores in the data.

We might take this a step further and have one initial loop that just finds the best score, then a second loop that just finds all the words having that specific score (rather than having a changing idea of what the best score is as we iterate). Then finally apply our tiebreaking logic if necessary.

It might feel like an approach that does everything in fewer loops will be more efficient, but our intuition here can be deceiving. We'll begin to see with Big O analysis that code that looks like it has to do more may be equivalent to (or even more efficient than) an all-in-one approach!

anselrognlie

These updates address the possible tiles in draw_letters, but there is still a small issue with get_highest_word_score that can lead to picking the wrong word under very particular conditions. Please take another look at get_highest_word_score.

anselrognlie · 2025-04-01T21:02:58Z

adagrams/game.py

+                if max_allowed > generated_dict[char]:
+                    generated_dict[char] += 1
+                    count += 1
+    generated_hand = [k for k, v in generated_dict.items() for _ in range(v)]


Nice use of a comprehension to expand the letters and counts out to a list containing the appropriate number of copies of each.

Consider using more descriptive variables in the comprehension, such as

generated_hand = [letter for letter, count in generated_dict.items() for _ in range(count)]

which would help describe a little bit more what's going on. Further, consider moving that to a helper function and calling it here, since this is a little difficult to read as a comprehension.

# elsewhere def expand_letter_counts_to_hand(leter_count_dict): return [letter for letter, count in leter_count_dict.items() for _ in range(count)] # then here generated_hand = expand_letter_counts_to_hand(generated_dict)

This tells us what's going on without the need to decipher the comprehension's intent. but if we need the details, we can go look in the function.

anselrognlie · 2025-04-01T21:33:20Z

adagrams/game.py

+            if len(ties) > 0:
+                ties.clear


👀 As written, this code has no effect on the tallying of the ties when a higher score word is found which still results in possible downstream problems. For instance, if I pass

["AAAAAAAAAA", "BBBBBB", "BBBBBBB", "GGGGGGG"]

for the words, I should get "BBBBBBB" as the winner (score 29), but I get "AAAAAAAAAA" (score 18) as the winner.

The fix for this is small and on this line, but I also encourage you to try tracing through how this particular set of inputs results in a failure.

anselrognlie · 2025-04-01T21:36:06Z

adagrams/game.py

+            if len(tied_words) < len(shortest_word):
+                shortest_word = tied_words
+                winner = shortest_word
+                # break


Prefer to remove logic over commenting it out. Commented logic left in code has a tendency to become uncommented as later devs come along and may get confused about whether the code was intentionally commented.

anselrognlie

Thanks for updating get_highest_word_score.

anselrognlie · 2025-04-26T18:23:30Z

adagrams/game.py

            if len(ties) > 0:
-                ties.clear
+                ties.clear()


👍 Yep. We needed to call the method. This resets the ties that we're tracking to only reflect the new highest score.

Notice that Python didn't give a squeek about us referencing a method without calling it. It was perfectly content to let us retrieve the method reference, and then basically throw it away without using it. So we need to be really careful of this in general. Adding additional tests (such as for the scenario I pointed out in my previous comment) that depend on the function being properly called can help us notice this.

rachanapatel added 4 commits March 13, 2025 17:22

testing 1

acb3d72

game file with func completed

52a176a

added helper func, fix readability

ebffcfc

cleaning up tiebreaker func

5fdbde6

anselrognlie reviewed Mar 25, 2025

View reviewed changes

rachanapatel added 2 commits March 31, 2025 12:44

updates to draw_letters func

8ab5d9b

updated get highest score func

435aaeb

anselrognlie reviewed Apr 1, 2025

View reviewed changes

updates to get highest score word score

af25053

anselrognlie reviewed Apr 26, 2025

View reviewed changes

		generated_dict[random_letter] += 1

		return generated_dict.keys()


		def get_highest_word_score(word_list: list) -> tuple[str, int]:
		score_tracker: dict[str, int] = {}


		# get highest value or add values to ties list
		for word_item, points in score_tracker.items():

Bumblebee - Rachana P #22

Are you sure you want to change the base?

Bumblebee - Rachana P #22

Uh oh!

Conversation

rachanapatel commented Mar 20, 2025

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!