Skip to content

Sphinx - Olga Karaivanska#38

Open
lioliadoc wants to merge 3 commits into
Ada-C22:mainfrom
lioliadoc:main
Open

Sphinx - Olga Karaivanska#38
lioliadoc wants to merge 3 commits into
Ada-C22:mainfrom
lioliadoc:main

Conversation

@lioliadoc

Copy link
Copy Markdown

No description provided.

Comment thread adagrams/game.py
@@ -1,11 +1,108 @@
import random
LETTER_POOL = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great constant variable naming convention here! 🎉

Comment thread adagrams/game.py Outdated
Comment on lines +34 to +35
letter_count = {letter: 0 for letter in LETTER_POOL}
letter_list = list(LETTER_POOL.keys())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Awesome use of comprehension here and use of the keys method and converting that into a list!

Comment thread adagrams/game.py
hand_of_letters.append(letter)
letter_count[letter] += 1

return hand_of_letters

@mikellewade mikellewade Sep 20, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great function, Olga! ⭐️ Only thing that's missing is the distribution. Read below:

Right now your code isn't actually utilizing the weight distribution of the letters. Currently, all letters have the same chance of being selected (they all can only appear their respective limits). randint is only randomly selecting from a pool of integers from 0 to 26. This means that Z is just as likely as likely to be selected as E. You would need it to select from a pool of integers from 0 to 98 to have the correct distribution. How could we change your letter pool to consider this weighted distribution?

Comment thread adagrams/game.py
def uses_available_letters(word, letter_bank):
pass
letter_bank_copy = letter_bank [:]
word = 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.

Nice work on ensuring that all the letters where of the same casing. You could also use casefold, which is used for caseless matching. You can find more information on the method here.

Also be mindful of re-assigning the parameter of a function. It can be a side effect of your function if the caller doesn't expect for the data that they passed in to be altered.

Comment thread adagrams/game.py Outdated
word = word.upper()
for letter in word:
for number, letters in scores.items():
if letter in letters:

@mikellewade mikellewade Sep 20, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Depending on how we structure our dictionary we could avoid this loop here. What do you think the change would be? When you learn about Big O this kind of thinking will come in handy.

Comment thread adagrams/game.py

if len(word) < len(winner[0]) or len(word) == 10:
winner = (word, score)
return winner

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very concise and readable function, praises! 🎉

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