-
Notifications
You must be signed in to change notification settings - Fork 47
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
Amber Lynn - Edges | API-Muncher #36
base: master
Are you sure you want to change the base?
Conversation
…Updated Api Wrapper
API MuncherWhat We're Looking For
This is a good start. I am definitely disappointed not to see tests - the work around mocking the external dependency with VCR is an important learning goal for this assignment, and not something we'll have the opportunity to come back to. Make sure you understand how and why we do that. I would also like to see more of a focus on error handling, this is a small thing that makes a surprisingly big difference in your development as an engineer. However, the main functionality of the site is there - searching and viewing details work great, and it is clear to me that the learning goals around working with an API and building non-model classes for a Rails app were met. It feels to me like most of the missing pieces are more a result of not having enough time than not understanding. Let me know if you disagree, or if there's specific places you'd like additional support, and keep up the hard work! |
if response.success? | ||
recipe = EdamamApiWrapper.build_a_recipe(response) | ||
return recipe | ||
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.
You should probably have an else
clause here, that either returns nil or raises an error of your own choosing (to be rescued in the controller).
|
||
def self.id_from_uri(uri) | ||
return uri.split("_")[1] | ||
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.
I like that you broke this logic out as a separate method - good organization.
def self.build_a_recipe(api_params) | ||
return Recipe.new( | ||
api_params[0]["label"], | ||
api_params[0]["image"], |
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.
This method and the previous one are very similar. Could you consolidate them somehow?
|
||
def show | ||
@recipe = EdamamApiWrapper.show_recipe( params[:id] ) | ||
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.
In these controller actions, you should check for an error response (empty array in index
, nil
or an exception in show
, and let the user know what happened, for example using a flash message.
|
||
get ':searches/index', to: 'searches#index', as: 'results' | ||
|
||
get 'searches/:id', to: 'searches#show', as: 'recipe' |
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.
Why is there a route parameter on line 6 - it looks like your search form always uses the word "search" here, and sends the search term via query params.
Could you have used resources :searches, only: [:index, :show]
here?
API Muncher
Congratulations! You're submitting your assignment!
Comprehension Questions
lib
? How would your project change if you needed to interact with more than one API (aka more than just the Edamam API)?