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

Shelan's API Muncher #43

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

Shelan's API Muncher #43

wants to merge 9 commits into from

Conversation

sheland
Copy link

@sheland sheland commented Nov 6, 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 utilized Postman where I looked at the JSON returned after inputting required parameters, app ID and Key to view what a recipe looks like. JSON for recipes is a giant array with hashes and arrays that contains recipe fields
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)? An API wrapper lets you call the API instead of the API directly. If you want to change from one API to another or if changes occur in the API, instead of rewriting your code, all you have to change is the wrapper. It it located in the 'lib' because rails doesn't automatically know about custom libraries lib folder. It does not have any magic around consuming APIs like it does for models or views.

Describe your API wrapper, the methods you created in it, and why you decided to create those methods/how they were helpful | I created 2 methods that list recipes and shows a recipe. I created 2 more methods ".create_recipe" and "self.keyboard" that creates a recipes and a recipe.
What was an edge case or failure case test you wrote for your API Wrapper? What was a nominal case? | I did not test edge or failure cases just nominal cases "it can list recipes' and "it can show a recipe"
How does VCR aid in testing an API? | It prevents you from calling the API by recording HTTP interactions and replay them .
What is the Heroku URL of your deployed application? | https://ada-muncher-2018.herokuapp.com/

@dHelmgren
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good, but more frequent commits are a good idea.
Comprehension questions You missed a piece on question 2! Otherwise good.
General
Rails fundamentals (RESTful routing, use of named paths) Good
Semantic HTML Yes
Errors are reported to the user Not well. See comments in the controller
API Wrapper to handle the API requests yes
Controller testing A little too basic. see comments
Lib testing See comments
Search Functionality yes
List Functionality yes, but see comments
Show individual item functionality yes, see comments
Styling
List view shows 10 items at a time and/or has pagination yes
Attractive and usable UI Simple, functional. Your page counters ended up in a weird spot though.
API Features
The App attributes Edamam main page does, but the other pages don't. 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 They, don't, good work!
External Resources
Link to deployed app on Heroku yes
Overall Good work, you covered the majority of the learning goals for this assignment. The most important thing that needs your attention is really the lack of attribution to Edamam, as that could lose you lots of money in the real world. Besides that, I REALLY want to encourage you to engage in test driven development as a part of this process: it takes more time up front, but when we start to run into issues as developers, knowing where the problems are from the tests saves a lot of time.

root 'recipe#home'

get '/recipes', to: 'recipe#index', as: 'recipes'
get '/recipe/:uri', to: 'recipe#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 the index and show pages?

)
end

def self.keyboard(api_params)

Choose a reason for hiding this comment

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

The keyboard method here seems like the thing you want to call when you do create_recipe as well. This code isn't very DRY.


def show
uri = params[:uri]
@recipe = EdamamApiWrapper.find_a_recipe(uri).first #takes the object out []

Choose a reason for hiding this comment

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

You've written your API wrapper to raise an error if the API call fails, but you're not looking for an error here. You should wrap this call in a begin/rescue.

it "should get index" do
VCR.use_cassette('search_file') do
get recipes_path
must_respond_with :success

Choose a reason for hiding this comment

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

There are some interesting cases you're not covering here:

  • What happens if no search term is provided?
  • What if the search returns no results?

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?


it 'can list recipes' do
VCR.use_cassette('seach_file') do #ensure code inside block will use cassette
hits = EdamamApiWrapper.search('food')

Choose a reason for hiding this comment

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

What if the search term has no results?

it 'can show a recipe'do
VCR.use_cassette('find_a_recipe') do
apple_pie = EdamamApiWrapper.find_a_recipe('pie')

Choose a reason for hiding this comment

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

What if the recipe doesn't exist?

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