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

Maddie Shields - Edges - API Muncher #48

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

Conversation

madaleines
Copy link

API Muncher

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How did you go about exploring the Edamam API, and how did you try querying the API? I read the documentation for Edamam API to learn how to use it and then tested it in Postman before applying URL's in my wrapper.
What is an API Wrapper? Why is it in lib? How would your project change if you needed to interact with more than one API (aka more than just the Edamam API)? I would change the params if I needed to use more than one API so it could call it once in the controller.
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful I have a search method which takes a query based on what the user is looking up, a show recipe method which takes a uri to return the recipe associated with that uri.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? If the recipe existed or did not for the show.
How does VCR aid in testing an API? It allows me not to make repeated calls on the api so I can minimize the use for cost reasons.
What is the Heroku URL of your deployed application? mads-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 - good work, keeping the routing RESTful is hard on this project!
Semantic HTML some - high level page structure (<header>, <footer>, <main>) is not clear
Errors are reported to the user no - A search with no hits results in a blank page, and mangling the URI on the show page results in an exception rather than a polite error message. Getting a good handle on error handling is not just good for your users, it will also make your life easier as a developer.
API Wrapper to handle the API requests yes - see inline comments
Controller testing yes
Lib testing yes
Search Functionality yes
List Functionality yes
Show individual item functionality yes
Styling
List view shows 10 items at a time and/or has pagination yes
Attractive and usable UI no
API Features
The App attributes Edamam no - This is really important! When you're designing code for money, using someone else's work without attribution is a good way to end up in legal trouble.
The VCR cassettes do not contain the API key yes
External Resources
Link to deployed app on Heroku Deployed, but can't search. Did you add your API keys as environment variables on Heroku?
Overall

Good work overall! The core functionality of this app works well, and it's clear to me that the learning goals of this assignment were met. Most of the things that aren't quite there (styling, deployment) feel to me like issues of not enough time rather than not understanding.

The one exception to this is error handling - make sure to pay close attention to this on future projects. Handling errors gracefully doesn't just help your users, it makes your life easier as well since you get clear info about why things aren't working. This will be especially important in JavaScript.

I've left a number of comments inline, but in general I am pretty happy with what I see here. Keep up the hard work!

recipe_uri_list << recipe.uri
end

if !recipe_uri_list.include?(uri)

Choose a reason for hiding this comment

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

This is clever - I like the idea of not doing another search if the query is in the most recent results, and I'm glad that you figured out how to make it work if the query is not.

We could probably clean this up a little though:

  def self.show_recipe(uri)
    # Check the most recent search results
    @recipe_list.each do |recipe|
      if recipe.uri == uri
        return recipe
      end
    end

    # Didn't find it, so make a new query
    recipe_uri_list = []
    # copy-paste lines 35-38, then end the method
  end


response = HTTParty.get(encoded_uri)
return Recipe.new(response[0])
end

Choose a reason for hiding this comment

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

What happens if the API call didn't return any results?

<!DOCTYPE html>
<html lang="en" dir="ltr">
<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 this HTML boilerplate in your view templates, it should be handled by layouts/application.html.erb.

it "should redirect to root with invalid id" do
VCR.use_cassette("show") do

get recipe_path ("000")

Choose a reason for hiding this comment

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

I love that you're testing this error case, but I don't think your application actually handles this situation.

describe "show_recipe" do
it "can show a single recipe" do
response = EdamamApiWrapper.search("chicken")
recipe_uri = "7bf4a371c6884d809682a72808da7dc2"

Choose a reason for hiding this comment

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

What if the recipe ID is invalid?

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