bengolden/hydrant_heart

Schema

hydrant_heart's Issues

Code Review

Code Review

@bengolden @albaer @common-nighthawk @msabdeljalil @4justinstewart @mikelikesbikes

Overall

• Thanks for adding the coveralls and Travis-CI widgets to the readme. Nice job with the 96% test coverage. It does look like you have failing tests though—at least one. Also pertaining to the README, it would be nice to provide instructions on getting the project up and running.
• It looks like a .DS_Store file snuck into your repo. Be mindful of your .gitignore file.
• Looks like some puts and console.log calls could be cleaned up.

DB / Migrations

• As we talked about in lecture this morning, foreign key fields and any other fields used regularly for look up should be indexed. I'm glad that you went with a postgresql database for development.

Models

Argument

• most_recent_first:
  Nice job using the ActiveRecord scope. I'm glad to see that this made it into your WelcomeController. After writing the tests for this with you all the other day, I was wondering if you would extract this logic from the controller and into the model. For robustness, you could consider changing this to a class method and passing the number of records to return as an argument. This would make the logic more reusable in different contexts. You could also pass an argument to the scope, but it seems to be preferred to write the method at that point.

• #users_supporting & #users_opposing:
  These could be written using ActiveRecord scoped has_many associations. Something like ...

  class Argument < ActiveRecord::Base
  
    has_many :supporting_votes, 
      -> { where "value = true" }, 
      class_name: "Vote", foreign_key: :voteable_id
  
    has_many :users_supporting, through: :supporting_votes, source: :user
  end
  

Claim

• #arguments_for, #arguments_against, #users_supporting & #users_opposing:
  As with some of your methods in Argument, I feel these could be associations.

User

• I'm not certain, but I believe that BCrypt will be in the environment, so you probably don't need to require it in Line 1. However, I think that you still need to include it in order to reference Password; alternatively, you could try not including BCrypt and instead reference BCrypt::Password. No guarantees, but since you had comments in the file asking whether or not the require and include statements were necessary, I thought I'd try to answer.

Vote

• You asked about validating the presence of a polymorphic association. I'm not sure what you mean by that. You could validate the presence of voteable_type as you're doing and then also voteable_id, but I don't think that's what you mean.

Controllers

ArgumentsController

• show:
  When you set @position, I assume it's to render the String "Supporting" or "Opposing"; that sort of simple logic can go in the view, in my opinion. Also, you don't need to create separate instance variables for the position, conclusion, author, etc. These can all be tied to @position (e.g. @position.conclustion or @positions.author. Eager loading, which Mike covered today, would give you this.
• new:
  It seems odd to automatically give a new argument three claims. I assume you're doing this to get three inputs in the form, but there are probably better ways of doing this than creating three empty objects.
• create:
  In Lines 32 -36, you're creating an argument and then assigning it some premises. I would move this logic into a helper method or two in the Argument model—probably including a #premises= setter method. Let the argument set itself up with its premises and then shovel that argument into current_user.authored_arguments.

ClaimsController

• show:
  Here we see the same over-creating of instance variables discussed with the ArgumentsController.
• new:
  I see that there are a few times where you want to redirect unless a user is logged in. You could consider using a filter.
• create:
  Make use of the ActiveRecord association class methods. You don't need to manually declare the author_id. You can shovel the new claim into current_user.authored_claims.

VotesController

• create:
  Make use of the ActiveRecord association class methods. It looks like this is setup only to work with AJAX requests.

WelcomeController

• authenticate:
  I would recommend moving this method to the User model.

JavaScript

• Not much here, but even so, I would recommend removing what is there from application.js and into its own votes.js file. Also, try to write object-oriented JS. The $(document).ready callback doesn't need to do contain the click listener. You could have something like ...

  var voteListener = {
    init: $('.vote_button').on('click', voteListener.callback),
    callback:  // some code
  }
  
  $(document).on("page:load", function() {
    voteListener.init(); // set up the listener
  }

CSS

• Again, not much here, but I would remove it from application.css/