-
Notifications
You must be signed in to change notification settings - Fork 44
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
API-muncher Brennan Octos #40
base: master
Are you sure you want to change the base?
Conversation
API MuncherWhat We're Looking For
This submission is concerning to me. You were able to get the core API functionality working, which is a good start. However there are many important pieces that are missing or broken, including error handling, testing, styling, git hygiene and deployment. Remembering how to write "regular" Ruby code is part of the challenge of this assignment, but it seems like you're still struggling with Rails and Ruby fundamentals. Many of the problems I'm seeing are things that will hamper the rest of your development process. For example, having a good story around error handling makes everything else easier, because when something goes wrong you get a clear reason why instead of having to hunt for it. When you meet with Charles this week, it would be worthwhile to go over some of these pieces, particularly error handling and testing. Aside from being important on their own, having these locked in a little better will help when we switch to JavaScript next week too. |
def index | ||
if params[:search] | ||
|
||
#returns all recipes for GET request from API |
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 the idea of using the same action/view for the homepage and search results.
|
||
<h1>Search recipes for </h1> | ||
|
||
<%= form_tag(root_path, method: 'get') 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.
You should put these in some sort of sectioning element - maybe a <header>
?
<% end %> | ||
|
||
<%= yield %> | ||
</body> |
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 always put the yield
inside a <main>
tag, since it's where the main content of your page will appear. Doing this will make debugging easier, since when you inspect the page it'll be clear whether content came from the layout or the view template.
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta charset="utf-8"> |
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 shouldn't be putting a full HTML document into your view templates, since they'll be inserted inside of views/layouts/application.html.erb
<main> | ||
<% @recipes.each do |recipe|%> | ||
<secton class = "recipe section"> | ||
<%= image_tag(recipe.image, alt: "picture of the 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 is good semantic HTML.
def catch_api_error | ||
begin | ||
|
||
yield |
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've copy-pasted this method here from the lecture notes, but you've commented out the around_action
above, and the error you're looking for doesn't match what your API wrapper is producing. It would be worth spending some time studying this to understand how it is supposed to fit together.
url = url_root + uri + "&app_id=#{APP_ID}" + "&app_key=#{APP_KEY}" | ||
|
||
response = HTTParty.get(url).parsed_response | ||
|
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 be checking the response for errors here.
# This is a factory method! It reads the data | ||
# we got back from the API, and turns it into | ||
# an instance of Channel by calling self.new | ||
|
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.
What is a factory method?
it "can get the index path" do | ||
get root_path | ||
|
||
must_respond_with :ok |
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 need a lot more in the way of controller tests. At the very least you need the following:
Index:
- A good search result is provided
- A search result is provided but produces no results
Show:
- A good recipe URI is provided
- A bad recipe URI is provided
You should also be wrapping these tests in VCR, since the controller make API calls via the API wrapper.
it 'can create an instance of itself' do | ||
a = EdamamApiWrapper.new | ||
a.wont_be_nil | ||
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.
Your application doesn't ever call new
on EdamamApiWrapper
- it only has self
methods and isn't meant to be instantiated. Why are you testing this functionality?
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.
Here is my list of test cases I'd look for in this file:
- Call
list_recipes
with a good search term, get back a list ofRecipe
objects - Call
list_recipes
with a bogus search term likeadlsfjaog
, get back an empty array or an error - Call
show_recipe
with a good URL, get back aRecipe
object - Call
show_recipe
with a bogus URL, get back an error ornil
API Muncher
Congratulations! You're submitting your assignment!
Comprehension Questions