-
Notifications
You must be signed in to change notification settings - Fork 464
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
Ruby - Laura Perez" #18
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! There is a bug in one of the functions which causes it to not work correctly. Please see the comments below about that and about places where the code can be cleaner/simpler. Excellent job on adding commits at least once each wave!
@@ -1,11 +1,144 @@ | |||
import random | |||
LETTER_POOL = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job using a helper variable!
adagrams/game.py
Outdated
# select letters (random) | ||
selected_letters = [] | ||
|
||
for i in range(0,10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since range starts at 0 by default, you do not need to specify it. You can just do for i in range(10)
.
adagrams/game.py
Outdated
index = big_string.index(letter_for_list) | ||
big_string = big_string[:index]+ big_string[index+1:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python has a built-in method for removing a character! It's called replace
. You can read more about it here: https://www.freecodecamp.org/news/how-to-remove-a-specific-character-from-a-string-in-python/
But this would look like this:
big_string = big_string.replace(letter_for_list, '', 1)
This means you want to replace the letter you picked with an empty string, but for only one instance of the letter.
adagrams/game.py
Outdated
letters = [] | ||
for k, v in LETTER_POOL.items(): | ||
letter_times = k * v | ||
letters.append(letter_times) | ||
big_string = ''.join(letters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
tests/test_wave_02.py
Outdated
@@ -35,6 +39,7 @@ def test_uses_available_letters_false_word_overuses_letter(): | |||
# Assert | |||
assert is_valid == False | |||
|
|||
@pytest.mark.skip(reason="no way of currently testing this") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure these are removed before you submit your code. None of the tests should be skipped!
adagrams/game.py
Outdated
|
||
def score_word(word): | ||
pass | ||
score_values = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have this declared outside of the function, like the LETTER_POOL
so that it does not have to be recreated each time the function is called.
adagrams/game.py
Outdated
word_upper = word.upper() | ||
lenght_of_word = len(word) | ||
total_score = 0 | ||
for letter in word_upper: | ||
if letter in score_values: | ||
total_score += score_values[letter] | ||
if lenght_of_word > 6: | ||
total_score += 8 | ||
|
||
return total_score |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Great job checking to make sure the letter is in the score_values
dictionary!
adagrams/game.py
Outdated
else: | ||
min_len = sorted_by_second[0][1] | ||
list_of_shortest = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of keeping the list of all of the words that have been the shortest, you can just keep track of the current shortest word.
adagrams/game.py
Outdated
if v <= value: | ||
print(f"value {value}") | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug here. The function will return True
right after checking just the first letter instead of the whole word. This code passes our tests, but would fail with other input. Try testing this function with the word being "AABB" and the letter_bank being ['A', 'A', 'B', 'C']. It will return True
even though it should return False
.
No description provided.