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

Pipes - Kee - API Muncher #28

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

Conversation

anemonekey
Copy link

API Muncher

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How did you go about exploring the Edamam API, how did you try querying the API? First, I read the documentation to get an idea of how the JSOn would look. Then, I tested it in the console and Postman application.
Describe your API Wrapper. How did you decide on the methods you created? I wrote two methods in the API Wrapper. The first one would list the search results for a query. The second would return the information of a specific recipe, given its unique ID credentials. I used these methods based on the requirements listed. Likewise, these were the only "options" for querying the Edamam DB.
Describe an edge case or failure case test you wrote for your API Wrapper. I wrote a failure case for API Wrapper when given an incorrect URI for a selected recipe to show. This will invoke an error.
Explain how VCR aids in testing an API. VCR was helpful because it meant I did not have to query the database every time I tested if my API calls were going through, saving precious bandwidth usage and time when initializing tests.
What is the Heroku URL of your deployed application? http://kee-muncher.herokuapp.com/

@droberts-sea
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Rails fundamentals (RESTful routing, use of named paths) yes
Semantic HTML yes
Errors are reported to the user some - when I mangled the URI for a recipe in the address bar, I got "We're sorry, but something went wrong."
API Wrapper to handle the API requests some - These methods could be expanded substantially - see inline comments.
Controller testing no
Lib testing yes - could be expanded, see inline comment
Search Functionality yes
List Functionality yes
Show individual item functionality (link to original recipe opens in new tab) yes
Styling
Foundation Styling for responsive layout yes
List View shows 10 items at a time/pagination yes
The app is styled to create an attractive user interface yes
API Features
The App attributes Edaman no - This is part of Edamam's terms of service! You should do it!
The VCR casettes do not contain the API key yes
External Resources
Link to deployed app on Heroku yes
Overall This is a good start. You've got an attractive and functional website that consumes the Edamam API, and it seems like most of the core learning goals were met.

I am a little concerned about your lack of test coverage, and that your error handling was quite minimal (especially in the search path). These two skills are closely linked - they both require you to be able to take a step back and think about your method as a unit: what does success or failure look like, what combinations of inputs might cause it to fail, and what should I do when it does fail. Writing code that's stable and resilient is not easy, but it's what separates a good application from a great one. Make sure you spend some time thinking about error handling and testing with VideoStore API, and let us know if you need more support on this.

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />

Choose a reason for hiding this comment

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

You shouldn't need to include all these <head> tags outside application.html.erb.

@query = params[:query]
@results = EdamamApiWrapper.search_recipes(@query)
if @results['hits'] != []
@results = @results['hits']

Choose a reason for hiding this comment

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

This work of parsing the response should probably be done inside the API wrapper. One of the big advantages of structuring the code that way is that it isolates your program's dependency on the API, but here that dependency is leaking out.

Lib code is similar to model code in that you should push as much functionality there as possible.


response = HTTParty.get(url)

return response.parsed_response

Choose a reason for hiding this comment

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

This method should be examining the response, checking for errors and returning a collection of Recipes, rather than the raw response.


if response.parsed_response == []
raise ApiError.new("Recipe does not exist")
end

Choose a reason for hiding this comment

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

I like this method a lot more, but there are still some error cases you're missing. The big one in my mind is when the API keys are not set up correctly, for example if a new teammate were to download your project and miss that step of setup.

response = EdamamApiWrapper.search_recipes(query)
response.must_be_kind_of Hash
response.length.must_be :>, 0
end

Choose a reason for hiding this comment

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

This is a good success case, but I would be interested to see what happens when you search for something that returns no results.

proc {
uri = "whatacrazyurihehehehehehe"
EdamamApiWrapper.show_recipe(uri)
}.must_raise EdamamApiWrapper::ApiError

Choose a reason for hiding this comment

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

Good failure case.

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.

2 participants