-
Notifications
You must be signed in to change notification settings - Fork 27
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
Katrina and Hayden's Adagrams #17
base: master
Are you sure you want to change the base?
Conversation
…onary are read properly
…enhance readability
AdagramsWhat We're Looking For
Hey y'all! Great work on this project. Your code fulfills all of the requirements and does it in a readable and reasonable way. The code is clean, and it passes all the tests! I'm making a few comments on this assignment about some suggestions on how to refactor, but they are all optional ways of looking at this code again. Overall, you two have done a great job. Good work! By the way, nice job with adding the tests for the optional wave! |
return true | ||
else | ||
return false | ||
end |
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.
Is there a way you could shorten this set of if/else
conditionals with a postfix conditional or a ternary?
it 'verifies that invalid word input returns false' do | ||
input = "poopydiscoopscoopdiddywhoopwhoopdiscoopdipoop" | ||
|
||
expect(is_in_english_dict?(input)).must_equal false |
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.
👍
expect(is_in_english_dict?(input)).must_equal true | ||
end | ||
|
||
it 'verifies that first entry in the dictionary CSV file is read properly' do |
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.
me being nitpicky: this test is not named accurately!
temp_letters_in_hand.delete_at(temp_letters_in_hand.index(letter.upcase)) | ||
letter_match_counter += 1 | ||
end | ||
end |
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.
Clever way of realizing that counting all "successful matches" with letter_match_counter
should be the same size as the input
!
I'll leave this as a "vague thing" to think about... I think that while you iterate through input.chars
, you can actually determine that uses_available_letters?
can return false
before the loop finishes.
Remember, when you use the return
keyword, it will kick the program execution out of the method early, and return whatever value you give it.
if word.include?(input) | ||
verify = true | ||
end | ||
end |
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.
Similar to a comment above but...
"Vague thing" to think about...While you iterate through dictionary
, you can actually determine that is_in_english_dict?
can return true
before the loop finishes.
Remember, when you use the return
keyword, it will kick the program execution out of the method early, and return whatever value you give it.
Adagrams
Congratulations! You're submitting your assignment.
Comprehension Questions
Enumerable
mixin? If so, where and why was it helpful?.include?
method several times to confirm whether a given array of letters (e.g.uses_available_letters?
andscore_word
) contained a particular letter. Much easier than iterating over each element of the array!binding.pry
several times to pause the program and test various variables at particular points in the file/loop/conditional/etc.