Skip to content

Spruce Symone and Cassie #38

Open
cohC16 wants to merge 11 commits intoAda-C16:masterfrom
cohC16:master
Open

Spruce Symone and Cassie #38
cohC16 wants to merge 11 commits intoAda-C16:masterfrom
cohC16:master

Conversation

@cohC16
Copy link
Copy Markdown

@cohC16 cohC16 commented Sep 24, 2021

No description provided.

Copy link
Copy Markdown
Collaborator

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Great work Symone and Cassie!

Your submission covers all the learning goals and passes all the tests! Overall, the code was clean and easy to read. This project is definitely worthy of a green grade 🟢 🌲✨

My feedback primarily focuses on ways to declutter functions, utilize dictionaries to improve performance, and maintain data types for variables. I also loved all the docstrings and comments, they were definitely helpful in understanding your thought process for these solutions.

Keep up the great work 🌲 ✨

Comment thread adagrams/game.py

# Hand draw logic, including incrementer and non-zero value check
while hand_size < 10:
letter = random.choice(list(letter_bank_keys))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
letter = random.choice(list(letter_bank_keys))
letter = random.choice(list(letter_bank))

The default iterator over a dictionary is its keys so the keys() method can be removed and we'll experience the same result.

Comment thread adagrams/game.py

def uses_available_letters(word, letter_bank):
pass
"""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Good use of the count method!

Comment thread adagrams/game.py
return True


def score_word(word):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Comment thread adagrams/game.py

# Checks len of word, if word is empty, and case of word
# in order to determine score
if len(word) in range(7, 11):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clever use of range to describes word lengths between 7 and 10.

Comment thread adagrams/game.py
Comment on lines +10 to +37
letter_bank = {
"A": 9,
"B": 2,
"C": 2,
"D": 4,
"E": 12,
"F": 2,
"G": 3,
"H": 2,
"I": 9,
"J": 1,
"K": 1,
"L": 4,
"M": 2,
"N": 6,
"O": 8,
"P": 2,
"Q": 1,
"R": 6,
"S": 4,
"T": 6,
"U": 4,
"V": 2,
"W": 2,
"X": 1,
"Y": 2,
"Z": 1,
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Long chunks of data like letter_bank tend to clutter functions and require more scrolling to see the rest of the function body. Consider moving this dictionary outside of this function (or in a new file) as a constant variable to be referenced in draw_letters.

Comment thread adagrams/game.py
"""

# Initialize variables
score_tuples = []
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This variable is reassigned to a tuple later but it remains unclear as to why score_tuples starts off as a list data type.

To stay true to its name and for clear communication purposes, I recommend keeping a variable assigned to a specific data type.

Suggested change
score_tuples = []
score_tuples = ()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is more of a python quirk, in statically typed languages like C#, assigning a variable to a different data type is NOT allowed (unless using some type of dynamic compiler).

Comment thread adagrams/game.py
Comment on lines +144 to +154
score_tuples = ((word, score_word(word)))

# If tied for high score
if score_word(word) == current_high_score:
if len(word) < winning_word_length and winning_word_length != 10:
score_tuples = ((word, score_word(word)))
winning_word_length = len(word)

# Length 10 edge case
elif len(word) == 10 and winning_word_length != 10:
score_tuples = ((word, score_word(word)))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The outer parenthesis is unnecessary.

Suggested change
score_tuples = ((word, score_word(word)))
# If tied for high score
if score_word(word) == current_high_score:
if len(word) < winning_word_length and winning_word_length != 10:
score_tuples = ((word, score_word(word)))
winning_word_length = len(word)
# Length 10 edge case
elif len(word) == 10 and winning_word_length != 10:
score_tuples = ((word, score_word(word)))
score_tuples = (word, score_word(word))
# If tied for high score
if score_word(word) == current_high_score:
if len(word) < winning_word_length and winning_word_length != 10:
score_tuples = (word, score_word(word))
winning_word_length = len(word)
# Length 10 edge case
elif len(word) == 10 and winning_word_length != 10:
score_tuples = (word, score_word(word))

Comment thread adagrams/game.py
winning_word_length = len(word)

# return high score as tuple
return score_tuples
Copy link
Copy Markdown
Collaborator

@audreyandoy audreyandoy Oct 11, 2021

Choose a reason for hiding this comment

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

Other than my comment on data types and parenthesis, this function looks great. We can really get bogged down with too many nested conditionals in this scenario so I appreciate how neat this approach is!

Comment thread adagrams/game.py
import random


def draw_letters():
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Comment thread adagrams/game.py
"""

# Created copy of letter_bank to leave original letter_bank unchanged
letter_bank_copy = letter_bank.copy()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good work 👍

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