Steven review

Notes.md

@@ -0,0 +1,12 @@
+* Top notch testing; THAT SAID: you'll find that most sites _do not_ test near
+  this much.  It's just too much labor sent to a non-visibly productive end.
+* This is a really solid app.  My main points here are is watch out for too
+  much calculation in the view.  In some places you're embedding a lot of
+  hidden tags or doing some string-ugly logic to get something to work.  I
+  think in most of those cases this was occasioned by dealing with the
+  polymorphic issues.  But in some of the cases, I think the thing you wanted
+  was available either in the session or in the params.  I can't be entirely
+  certain on all of those, but keep things open.
+* Your routes and controllers were very nice.
+* Very solid project that shows Rails is on your side as yu go into final
+  projects.

app/controllers/answers_controller.rb

@@ -2,13 +2,16 @@ class AnswersController < ApplicationController
   before_action :require_logged_in, only: [:new, :create, :update, :destroy]
   def create
     @answer = Answer.new(answer_params)
+    # You can bolt this calculation onto answer_params using .merge()
     @answer.user_id = current_user.id
     if @answer.save
       username = User.find(@answer.user.id).username
+      # Typically we see this being in-lined into the following line
       data = {answer: @answer, username: username}
       render json: data.to_json
     else
       flash[:alert] = @answer.errors.full_messages
+      # Typically render is the last thing
       render json: {error: "Didn't work"}.to_json
       @answer.user_id = answer_params["user_id"] || current_user.id
     end
@@ -19,6 +22,8 @@ def update
     @answer.assign_attributes(answer_params)
     if @answer.save
       flash[:notice] = "Answer successfully updated."
+      # I dislike going :back, send me somewhere clear, please.  It's a style
+      # thing but that's my opinion.
       redirect_to :back
     else
       flash[:alert] = @answer.errors.full_messages

app/controllers/comments_controller.rb

@@ -1,5 +1,6 @@
 class CommentsController < ApplicationController
   before_action :require_logged_in, only: [:new, :create, :update, :destroy]
+  # See how I cleaned this up on Wed nite lecture
   def create
     @comment = Comment.new(comment_params)
     @comment.user_id = current_user.id

app/controllers/questions_controller.rb

@@ -5,6 +5,7 @@ def index
   end
 
   def show
+    # This just makes me nervous.  WHY DO YOU NEED SO MUCH DATA?
     @question = Question.find(params[:id])
     @answers = @question.answers.all
     @comment = Comment.new

app/controllers/votes_controller.rb

@@ -4,6 +4,8 @@ def new
     @vote = Vote.new
   end
 
+  # Obviously this could be really heavily refactored....
+  # As a baseline methods should be about 5-10 lines...
   def create
     @old_vote = Vote.find_by(user_id: current_user.id, voteable_id: vote_params["voteable_id"], voteable_type: vote_params["voteable_type"])
     if @old_vote && @old_vote.value != vote_params["value"]

app/views/comments/_answer_comment.html.erb

@@ -1,5 +1,7 @@
+<%# this id thing is just so ugly.  since this is a partial couldn't you pass in something from the parent that doesn't require this ugly string concatenation %>
   <%= form_for @comment, :html => { :class => "answer-comment-form", :id => "answer-comment-" + answer.id.to_s, style:"display:none;" } do |f| %>
     <%= f.text_area :body, size: "100x5" %>
+    <%# if it should be there, don't be a wimp: just call .id %>
     <%= f.hidden_field :user_id, value: current_user.try(:id) %>
     <%= f.hidden_field :commentable_type, value: "Answer" %>
     <%= f.hidden_field :commentable_id, value: answer.id %>

app/views/questions/_answer.html.erb

@@ -1,6 +1,7 @@
 <div class = "answer-partial" id = "question-info">
   <p><%=answer.body%> </p>
   <p><%=answer.user.username%></p>
+  <%# while you'e inlined nicely this is doing too much thinking in a view... %>
   <p>Answer Vote count: <span id="vote-count-answer-span"><%= answer.votes.pluck(:value).reduce(:+) || 0%> </span></p>
   <%= render partial: "/votes/answer_vote", locals: {answer: answer, index: index} %>
-</div>
+</div>

config/routes.rb

@@ -1,5 +1,6 @@
 Rails.application.routes.draw do
 
+  # Stunning how concise but powerful this can get, no?
   resources :users
   resources :questions
   resources :answers

spec/models/answer_spec.rb

@@ -9,6 +9,8 @@
     expect(FactoryGirl.create(:answer)).to respond_to :question
   end
 
+  # Typically these low-level tests of associations are not tested (in my
+  # experience).  Maybe they should be, FYI.
   it 'should respond to Comments' do
     expect(FactoryGirl.create(:answer)).to respond_to :comments
   end