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

Refactor User model #86

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

Refactor User model #86

wants to merge 7 commits into from

Conversation

johnktravers
Copy link
Owner

What does this PR do?

  • Replaces the worst_ingredients and best_ingredients methods in the User model with a sorted_ingredients method that takes flag and count arguments
  • Replaces the worst_ingredient_data and best_ingredient_data methods with a to_data class method in the Ingredient model
  • Updates tests accordingly (test coverage is still at 99.9%)
  • Configures static assets using an Amazon Cloud Services CDN (unrelated - sorry for mixing this in)

Where should the reviewer start?

  • With the sorted_ingredients method in the User model
  • Then look at the to_data method in the Ingredient model

Thoughts/Refactors

  • It occurred to me that sorted_ingredients is never called without mapping the ingredients to a hash with the to_data method, so just having one method that sorts the ingredients behind the scenes and returns the hash might be preferable
  • I would love to hear your thoughts on how to improve this refactor!

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.

1 participant