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

Amber Lynn - Edges | API-Muncher #36

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

Conversation

griffifam
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 compared json to slack api to notices how things were structured within hashes and arrays.
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'd have to create a new wrapper. And it is in lib because it follows its own conventions and is not a part of the db
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful The API wrapper is what packages the parsed json data and returns it to controller. The wrapper talks to API
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? I have not written tests
How does VCR aid in testing an API? VCR records new episodes (responses) and plays back old responses
What is the Heroku URL of your deployed application? I have not deployed

@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) see inline
Semantic HTML some - missing high-level elements (<header>, <footer>, <main>)
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
Controller testing no
Lib testing no
Search Functionality yes
List Functionality yes
Show individual item functionality yes (missing details, but the page is there)
Styling
List view shows 10 items at a time and/or has pagination no
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 N/A
External Resources
Link to deployed app on Heroku no
Overall

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

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

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"],

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

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'

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?

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