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

Marlyn Lopez - Luz Andrea Garcia Zapata /C17 - Otters Class #53

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrygzt
Copy link

@andrygzt andrygzt commented Apr 1, 2022

No description provided.

Copy link

@kendallatada kendallatada left a comment

Choose a reason for hiding this comment

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

Hi Marlyn and Andrea! This has been scored as yellow due to the logical errors in two of your functions. Please check your code to see the comments I left. Let me know if you have any questions. Nice work overall! :)


import random

#from constants import *

Choose a reason for hiding this comment

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

Careful not to leave unused code hanging around. It looks like adagrams/constants.py and tests/constants.py are not being used. I know neither of these modules are being used, but try to avoid duplicate code. These modules are exactly the same, they just live in different directories. So, make sure to only have one copy of a module that gets imported by all other modules / tests that may need it.

def draw_letters():
pass
letter_pool = LETTER_POOL.copy()

Choose a reason for hiding this comment

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

Nice use of the copy method to avoid modifying the LETTER_POOL constant ✨

upper_word = word.upper()
for i in upper_word:
counter += 1
if upper_word.count(i) == letter_bank.count(i) and counter == len(upper_word):

Choose a reason for hiding this comment

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

Love your usage of the count method! 💕 I would reconsider how you have structured the conditionals in this loop though.

  1. This function passes all of the provided tests but there are some logical errors that will lead to incorrect behavior. Consider if you call the function with these arguments: uses_available_letters("cog", ["D", "O", "G"]) the function will return True. Think about why that is and how you can update your code to solve this logical error.
  2. If we only want to return True/False after checking all letters in the word, then we don't need to check if we're at the end of the word every loop iteration. Think about how you can accomplish this without checking this conditional each loop iteration. With this new approach, is the counter necessary?
  3. It's possible to implement this solution with only 1 conditional statement. Think about how you could accomplish that.

# allow user enter upper and lower case letters
upper_word = word.upper()
score = 0
for letter in upper_word:

Choose a reason for hiding this comment

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

Beautiful solution 🥳


return score



def get_highest_word_score(word_list):

Choose a reason for hiding this comment

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

Beautiful solution 🥳


if len(temp_word) == len(word):
best_word = word_list[0]

Choose a reason for hiding this comment

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

Make sure to read the requirements carefully and that your interpretation aligns with the overall goal. The wording in the README for this specific case could be interpreted as return the first item in the list if there are two items of the same length and the same score. However, if we go with that interpretation and we run the function with this input:
get_highest_word_score(["A", "AAAAAAAAAA", "EEEEEEEEEE"])
The output will be: ('A', 18)
'A' is not the word with the highest score and 18 is not the correct score for the word 'A'. I would categorize this as a logical error. Think about how you could update your code to handle this case correctly.

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.

3 participants