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

C19Ruby- Sarah Kim #19

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

C19Ruby- Sarah Kim #19

wants to merge 11 commits into from

Conversation

suyon02kim
Copy link

No description provided.

Copy link

@mmcknett-ada mmcknett-ada left a comment

Choose a reason for hiding this comment

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

Looks great! 🟢 Green!

The tests all pass, and your code is concise and readable. I have some nitpicks, comments, and suggestions, but those don't detract from the fact that this is a solid submission.

Other overall comments:

  • Don't forget to make your PRs against Ada-C19 instead of AdaGold
  • You commit a lot with good messages; keep it up! Committing this frequently will help you so much later on.
  • Very nice and concise implementation of get_highest_word_score!

adagrams/game.py Show resolved Hide resolved
}

while len(list_of_letters) < 10:
letter_chosen = random.choice(list(LETTER_POOL))

Choose a reason for hiding this comment

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

This way of choosing letters randomly doesn't quite meet the requirements. Notice that your solution is just as likely to draw a Z as it is to draw an E. The requirements say, "Since there are 12 Es but only 1 Z, it should be 12 times as likely for the user to draw an E as a Z". It turns out, our tests don’t test for that. (That test is quite hard to write!) Think about how you might modify your code to fulfill this requirement.

Regarding random.choice: when bringing in functions from imported modules (that we haven’t covered in the curriculum), please consider including a comment about how you understand the function to work, and how you researched it. Now that we’ve discussed Big O, it’s also useful to think about what the time/space complexity of the function might be. Thinking about how we can implement this functionality ourselves can help us gain insight into what that might be.

Remember that not every language has the same built-in functions, so we should be able to implement this same functionality ourselves. In the projects, we don’t ask for any features that we don’t think you should be able to write yourself. For drawing a random hand, the only external function that would be “required” is a way to pick a random number (such as randint). At this stage in our coding journeys, it’s often more valuable to re-implement things by hand to practice thinking about how to break down and approach the problem than to use a single library function to solve the problem for us.

Comment on lines +52 to +59
for elem in word: # loop through each letter in the word
if elem in letter_frequency.keys():
if letter_frequency[elem] > 0:
letter_frequency[elem] -= 1
else:
return False
else:
return False

Choose a reason for hiding this comment

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

Nice way of handling this!

[suggestion] You can make the cases slightly more concise by inverting the conditions, combining them, and early returning only in the if body. Early-returning in an if means the rest of the loop body is naturally an "else":

    for elem in word: # loop through each letter in the word 
        if elem not in letter_frequency.keys() or letter_frequency[elem] <= 0: 
            return False 
        letter_frequency[elem] -= 1


def score_word(word):
pass
letter_score = {

Choose a reason for hiding this comment

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

[nit] since you don't modify letter_score, it can be extracted out of the function as a constant. You might call it LETTER_SCORE if you did that.

"Z" : 10
}
total_score = 0
#turn every letter in the word to upper case

Choose a reason for hiding this comment

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

Comments are good, and sometimes you can have too much of a good thing! In most of this function, your comments are almost exactly the same as the lines of code they document. We avoid that in industry because the lines of code may change, but devs aren't always careful about changing the comments to match. It's also more for humans to read, but doesn't give them extra information.

What comments might you write to describe what this code is doing without describing how it does it (since that is what's most likely to change later)?

Comment on lines +111 to +117
# length of 10 letters trumps any word that is shorter
if len(new_word) == 10 and len(highest_scoring_word)!= 10:
highest_scoring_word = new_word
# having fewer letters is a tie breaker as long as the current stored word's length is not 10
elif len(highest_scoring_word) > len(new_word) and len(highest_scoring_word) != 10:
highest_scoring_word = new_word

Choose a reason for hiding this comment

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

[idea] If you think it's more readable (which you might not!), you can combine these cases together and rely on the order of operations rules of or and and to give you equivalent boolean logic:

        if score_for_one_word == highest_score and len(highest_scoring_word) != 10:
            if len(new_word) == 10 or len(highest_scoring_word) > len(new_word):
                highest_scoring_word = new_word

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