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

Adagrams - Valerie & Anibel - Edges #13

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

Conversation

anibelamerica
Copy link

Adagrams

Congratulations! You're submitting your assignment.

Comprehension Questions

Feature Feedback
What are the components that make up a method? The components that make up a method are the method signature and the code block to be executed.
What are the advantages of using git when collaboratively working on one code base? Some advantages to using git when working collaboratively are being able to work on separate machines and providing version control.
What kind of relationship did you and your pair have with the unit tests? We generally found them useful, but we also realized that just because tests pass, doesn't mean the code is bug free.
Does your code use any methods from the Enumerable mixin? If so, where and why was it helpful? Yes, our code uses the .min_by and .select methods to apply tie-breaking rules in the highest_score_from method. These methods were helpful because we could apply the tie-breaking rules with fewer lines of code and increase readability.
What was one method you and your pair used to debug code? We used the rake file to run tests and then ran the driver code for each wave. Using both methods allowed us to check for bugs more thoroughly.
What are two discussion points that you and your pair discussed when giving/receiving feedback from each other that you would be willing to share? We discussed that going forward we should have better break-taking hygiene. We also discussed how we were both worried we would be too controlling but neither believed this to be true.

@tildeee
Copy link

tildeee commented Aug 24, 2018

Adagrams

What We're Looking For

Feature Feedback
General
Answered comprehension questions x
Both teammates contributed to the codebase x
Small commits with meaningful commit messages commit messages would be better if they were more descriptive instead of "implements wave X"
Code Requirements
draw_letters method x
Uses appropriate data structure to store the letter distribution x
All tests for draw_letters pass x
uses_available_letters? method x
All tests for uses_available_letters? pass x
score_word method x
Uses appropriate data structure to store the letter scores x
All tests for score_word pass x
highest_score_from method x
Appropriately handles edge cases for tie-breaking logic x
All tests for highest_score_from pass x
Overall

Hi all!

Wonderful submission for this project! I found the code readable and hitting all the requirements, but I also found some parts to be really clever and clean. I'm really impressed!

Some lines of code show that you two clearly have a good grasp on how conditionals evaluate, how to compare and reassign, and how to use Enumerable methods, because a lot of the code was refactored and terse in a good way.

Also, great job on the tests you added for the optional wave!

Overall, good job. I don't have too many comments on this submission, so let me know if you have any questions


end

return false
Copy link

Choose a reason for hiding this comment

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

Always returning false if it gets to this point (aka hasn't returned true beforehand) is great!


if score == best_word[:score]
best_word[:word] << word
end
Copy link

Choose a reason for hiding this comment

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

I really like this approach -- shoveling word into best_word hash on a tied score, or clearing it if the score is clearly better

end
end

return input_array.length == 0
Copy link

Choose a reason for hiding this comment

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

I love this -- returning true or false based off of this evaluation.

I think you could still have added above this

if input_array.include? letter
  ...
else
  return false
end

so that the method could jump out of the iteration sooner than looping all the way through, but yeah, this is great!


expect(is_in_english_dict?('niugfdfl')).must_equal false
expect(is_in_english_dict?('hippopotamus')).must_equal false
expect(is_in_english_dict?('#&*123')).must_equal false
Copy link

Choose a reason for hiding this comment

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

these are all really good edge cases that I would expect! Nicely done

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