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

API-Muncher Marisa Morris #33

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

API-Muncher Marisa Morris #33

wants to merge 25 commits into from

Conversation

marisamorris
Copy link

@marisamorris marisamorris commented Nov 6, 2017

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? Returning all of the data and then used nesting to step into to main hash further.
Describe your API Wrapper. How did you decide on the methods you created? My API wrapper contains two methods. A list_recipes method and a show recipe method. As I only needed to display a list of recipes and show an individual recipe, I didn't see the need for any additional. One change i could have made would have been to create a method that pulls the pieces of data i need so that each method can have a single responsibility.
Describe an edge case or failure case test you wrote for your API Wrapper. Will return [] for a broken request. If someone searches for nothing, an empty array is returned.
Explain how VCR aids in testing an API. VCR is a tool that allows your tests to record HTTP interactions and replay them when necessary
What is the Heroku URL of your deployed application? http://theguiltycarb.herokuapp.com/

@CheezItMan
Copy link

API Muncher

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commit and commit messages
Comprehension questions Check, I would also emphasize that VCR keeps you from running too many api calls
General
Rails fundamentals (RESTful routing, use of named paths) Check
Semantic HTML You've got a few too many div elements
Errors are reported to the user Wrong show URLs crash the app, otherwise no errors reported
API Wrapper to handle the API requests Check, but more defensive programming needed
Controller testing Check, but no negative-case tests
Lib testing Check, but no negative-case tests
Search Functionality Check
List Functionality Check
Show individual item functionality (link to original recipe opens in new tab) Check
Styling
Foundation Styling for responsive layout Your site simply shrinks the results as the browser gets smaller. This results in an unpleasant experience on a mobile device. Using grid-layout would be better. Something to work on in JavaScript!
List View shows 10 items at a time/pagination Nice work!
The app is styled to create an attractive user interface The styling is pretty nice.
API Features
The App attributes Edaman Attribution is there, but no link
The VCR casettes do not contain the API key Check
External Resources
Link to deployed app on Heroku Check
Overall Overall well done. You had some oversights in testing which expose problems in your app. I left some notices in your code. Overall not bad.


describe "show_recipe" do

it "Can show a requested recipe" do

Choose a reason for hiding this comment

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

What about a negative case?

@@ -0,0 +1,8 @@
require "test_helper"

class HomepageControllerTest < ActionDispatch::IntegrationTest

Choose a reason for hiding this comment

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

This should probably be removed.

require 'test_helper'

# class RecipesControllerTest < ActionDispatch::IntegrationTest
describe RecipesController do

Choose a reason for hiding this comment

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

Again negative-case tests are needed!

<div class="grid-x">
<div class="large-2 medium-1 small-3 cell"></div>

<div class="large-8 medium-10 small-6 cell blah">

Choose a reason for hiding this comment

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

blah?

# MUST ENCODE THE SEARCH PARAMETER IN THE URL
url = BASE_URL + "?r=" + (URI.encode(search))

data = HTTParty.get(url)

Choose a reason for hiding this comment

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

You need some error-detection here! What if data or data[0] are nil?

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