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

Caroline Nardi - API Muncher - Octos #29

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

Conversation

cmn53
Copy link

@cmn53 cmn53 commented May 7, 2018

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? I looked through the documentation and then sent a general query request and a specific recipe request in Postman to explore the response structure.
Describe your API Wrapper. How did you decide on the methods you created? My API wrapper has two methods, one for calling the Edamam API with a general query and returning a collection of recipes, and one for requesting a single recipe from the API and returning an instance of class Recipe. I decided on two methods because I felt the parameters for each API call differed enough to necessitate distinct methods.
Describe an edge case or failure case test you wrote for your API Wrapper. I wrote edge case tests for invalid or absent queries or recipe ids (although I wasn't successful in parsing out the API's response codes, so these all display the same generic error message whether the call fails or there just aren't any results from the query.
Explain how VCR aids in testing an API. VCR records HTTP responses so that the tests do not have to make calls directly to the API. This helps you to not rack up unnecessary API calls during testing or have failing tests if the API is down.
What is the Heroku URL of your deployed application? https://adie-wok-in-progress.herokuapp.com/
Provide a link to the Trello board you used https://trello.com/b/RrI9Pe3V/api-muncher-public

@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 yes
API Wrapper to handle the API requests yes
Controller testing yes
Lib testing yes
Search Functionality yes
List Functionality yes
Show individual item functionality (link to original recipe opens in new tab) yes
Styling
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 yes
The VCR casettes do not contain the API key yes
External Resources
Link to Trello Board yes
Link to deployed app on Heroku yes
Overall Excellent job overall! This code is well-organized and easy to read, and it's clear the learning goals for this assignment were met. I've left some nitpicks inline below, but in general I'm quite happy with this submission. Keep up the hard work.

# For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html
root 'recipes#search'
get '/results', to: 'recipes#index', as: 'results'
get '/recipes/:id', to: 'recipes#show', as: 'recipe'

Choose a reason for hiding this comment

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

Could you use resources for this?

def catch_api_error
begin
yield
rescue EdamamApiWrapper::EdamamError

Choose a reason for hiding this comment

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

Good work matching your error handling here to what's going on in the API wrapper.

APP_ID = ENV["EDAMAM_APP_ID"]
APP_KEY = ENV["EDAMAM_APP_KEY"]
BASE_URL = "https://api.edamam.com/search?"
BASE_URI = "http://www.edamam.com/ontologies/edamam.owl#recipe_"

Choose a reason for hiding this comment

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

The variable names BASE_URL and BASE_URI are very similar - I would be worried about typoing one for the other. Could you choose something more visually distinct (maybe BASE_RECIPE_ID)?


unless response["count"] > 0
raise EdamamError.new
end

Choose a reason for hiding this comment

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

You should include an error message here, so that if some other developer (or future you) encounters this error they know what went wrong.

describe "index" do
it "succeeds for a valid search query" do
VCR.use_cassette('recipes') do
get results_path, params: { query: "chicken"}

Choose a reason for hiding this comment

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

It might also be worthwhile to add some tests around the paging parameters:

  • What happens if they're absent?
  • Do you get different results when they change?
  • What if you send a bogus value, like a negative page number?

@droberts-sea
Copy link

Also, I love your site name.

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