Ana Lisa Sutherland: API Muncher -Octos C9 #34
Conversation
…want only 10 returned or all of the recipes returned found and paginate 10 per page
…to fake a database and create tests.
API MuncherWhat We're Looking For
This submission is concerning to me. You were able to get the core API functionality working, which is a good start. However there are many important pieces that are missing or broken, including error handling, testing, routing, styling, and deployment. Remembering how to write "regular" Ruby code is part of the challenge of this assignment, but it seems like you're still really struggling with Rails and Ruby fundamentals. Many of the problems I'm seeing are things that will hamper the rest of your development process. For example, having a good story around error handling makes everything else easier, because when something goes wrong you get a clear reason why instead of having to hunt for it. When you meet with Charles this week, it would be worthwhile to go over some of these pieces, particularly error handling and testing. Aside from being important on their own, having these locked in a little better will help when we switch to JavaScript next week too. I know you've been putting in a ton of effort recently, and I want to acknowledge that. I do see improvements happening and progress being made. Keep up your focus and dedication, and things will start falling into place. |
| <h4><%= recipe.label %></h4> | ||
| </li> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
This </div> should be inside the loop.
| <div class= "recipe_info"> | ||
| <li class= "recipe_img"> | ||
| <%= link_to image_tag(recipe.image, alt: "recipe-pic", class: "recipe-pic"), recipe_path(uri: recipe.uri) %> | ||
| </li> |
There was a problem hiding this comment.
You should invert the <li>s and <div>s here, have the children of the <ul> be one <li> per recipe, and have two <div>s within that.
lib/edemam_api_wrapper.rb
Outdated
| url = BASE_URL + "/search?r=#{encoded_uri}&app_id=#{API_ID}&app_key=#{TOKEN}" | ||
| response = HTTParty.get(url) | ||
|
|
||
| return Recipe.format_api(response[0]) |
There was a problem hiding this comment.
You don't have any error handling here. What if the user types in a recipe that doesn't exist?
| def initialize(label, options = {}) | ||
| if label.nil? || label.empty? | ||
| raise ArgumentError.new("No name has been provided for this recipe") | ||
| end |
There was a problem hiding this comment.
Since you need the uri to make the links, you should probably include a check for that here.
test/lib/edemam_api_wrapper_test.rb
Outdated
| recipes = EdemamApiWrapper.search_recipes(query) | ||
|
|
||
| get recipes_path, params: {query: "pepper"} | ||
| must_respond_with :success |
There was a problem hiding this comment.
You both make a request and call the API wrapper directly in this test. You should split these into separate controller and lib tests.
test/lib/edemam_api_wrapper_test.rb
Outdated
| recipes = EdemamApiWrapper.search_recipes("pepper") | ||
| recipe = recipes.first | ||
|
|
||
| get recipe_path(recipe.id) |
There was a problem hiding this comment.
On line 37, you're using a named path, but these aren't defined in your routefile, which is why this fails.
config/routes.rb
Outdated
|
|
||
| get 'search', action: :index, controller: 'recipes' | ||
| get 'index', to: 'recipes#index' | ||
| get '/recipe', to: 'recipes#show' |
There was a problem hiding this comment.
These routes are not RESTful! That makes the pattern harder to remember, making it more likely that you'll write a bug. You also don't have path names, which means you have to type out the URL as a string everywhere. Instead you should use resources :recipes, only: [:index, :show].
API Muncher
Congratulations! You're submitting your assignment!
Comprehension Questions
Just realized I forgot to have the recipe source url reopen in a new tab, got too bogged down trying to get tests to work....