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

Naheed Edges #37

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

Naheed Edges #37

wants to merge 13 commits into from

Conversation

arangn
Copy link

@arangn arangn commented Nov 5, 2018

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 and experimented on postman
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)? The wrapper connects the API and the controller. Since it isn't a model or a controller, it is in the lib file. And to interact with multiple APIs, you would make multiple wrappers
Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful I created a list recipes method to list all the recipes in the index, a create recipes method, and a show recipe for the show route.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? I wrote an edge case that it would return an empty array for an invalid search. A nominal case was that a recipe matched the correct recipe uri
How does VCR aid in testing an API? It doesnt call the API every time it runs the test
What is the Heroku URL of your deployed application? (its ugly im sorry 😭) https://recipe-muncher-naheed.herokuapp.com/

@Hamled
Copy link

Hamled commented Nov 14, 2018

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Mostly, this could have been broken up into smaller commits.
Comprehension questions
General
Rails fundamentals (RESTful routing, use of named paths)
Semantic HTML Somewhat.
We could definitely have more structure and use more semantic tags than just section.
Errors are reported to the user
API Wrapper to handle the API requests
Controller testing No.
Lib testing
Search Functionality
List Functionality
Show individual item functionality
Styling
List view shows 10 items at a time and/or has pagination
Attractive and usable UI Sort of. I'm not here to judge anyone's design, but this is definitely... minimal.
API Features
The App attributes Edamam No.
The VCR cassettes do not contain the API key
External Resources
Link to deployed app on Heroku
Overall Decent implementation on all the important bits -- the API wrapper code and associated tests. Would have been nice to have tests for the controller actions as well, though.

describe "create_recipe method" do
it "creates a valid recipe" do
VCR.use_cassette('list_recipes') do
response = EdamamApiWrapper.list_recipes("apple")
Copy link

Choose a reason for hiding this comment

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

I think we don't actually need this test for EdamamApiWrapper.create_recipe.

This is because it is a private method, so we can't call it directly. Indeed, the test code is instead calling list_recipes, which we know does eventually call create_recipe.

Since we can't directly call the method, it's not something we want to have explicit tests for -- this test is basically the same as the one on line 7, so we can remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants