gistfile1.md

Code Review

• 'Helper' is a code smell
  • "helper" means "I couldn't think of a better name and/or organization for this class hierarchy."
  • See Snippet 1 and Snippet 1 Refactored
• HashWithIndifferentAccess is a code smell
  • https://groups.google.com/forum/?fromgroups#!msg/newhavenrb/L3E5uFthfAo/sN62NtXYMxcJ

    symbols are not garbage collected.

  • Promotes sloppy coding. Prevents developers from learning the difference between strings and symbols.
  • If you're hashing user input, it should be a string, not a symbol.
  • When you need to go from a string to a symbol, it is often the case that you need to do some security checks. A hash with indifferent access makes it too easy to skip those checks.
  • example: session

    always use strings with session. they are being stored as strings on the client. we're being n00bs trying to pretend everything is a symbol.

• how to treat params
  • every object in our code should either be 1) untrusted user data, or 2) trusted objects created by us. When params is written to, we have some kind of bastard child of both, which is too complicated. thus params should be read-only.

    code base currently has 357 violations of this principle.

  • whenever you're dealing with params, the goal should be to validate the data as soon as possible so we can work with trusted objects, not untrusted user data. we shouldn't ever have references to params buried in hepers or something that will get included.
  • never mass-assign with params.
• before_filter
  • Good: Filters that tag actions in various ways:

    redirect_to_ssl admin_required login_required sudo_password_required

  • Bad: Filters that do things. These should just be methods.

    set_session_signup_data store_return_to_or_redirect setup_user

• default parameters
  • makes refactoring harder
  • makes intent less clear
  • leads to DRY violations
• legacy url support

  • instead of something like declaring a helper called promo_code_name which does params[:promo_code] || params[:id] and using it everywhere, just use the canonical value.

    Let's have a LegacyUrlController whose entire job is to serve a redirect to the new URL, or at least modify the parameters and pass the request off to the new controller.

  • when you move params parsing logic to a helper and start using it everywhere, you make refactoring difficult, because you have created a bunch of extra URLs that it is now a question of whether to support.

• method calling convention
  • when you look at ruby code that is calling a function with no arguments, it is ambiguous whether it is calling a function, or accessing a variable.
  • this makes reading the code more difficult
  • when no arguments, let's call things like this: some_method()
• size of applicationcontroller
  • see Snippet 2
  • every single controller has all that code. instead of extending ApplicationController, create a superclass for the controller to stick in between: MyController < CreateSomeIntermediateClassLikeThis < ApplicationController then another MyController2 can inherit from CreateSomeIntermediateClassLikeThis but every other controller doesn't have to know about the methods.

Snippet 1:

module PromoCodesHelper

  def create_product_order_for_promo_code_param_and_user(user = User.new)
    return unless promo_code.present?

    @product_order = ProductOrder.create!(:user => user)
    @product_order.promo_code = promo_code
    @product_order.save!
  end

  def promo_code_name
    params[:promo_code] || params[:id]
  end

  def promo_code
    begin
      PromoCode.from_param(promo_code_name)
    rescue ActiveRecord::RecordNotFound
      nil
    end
  end

end

Snippet 1 Refactored:

class ProductOrder < ActiveRecord::Base
.
.
.
  def self.create_from_promo_code_and_user!(promo_code, user)
    order = self.create!(:user => user)
    order.promo_code = promo_code
    order.save!
    order
  end

  def self.create_from_promo_code_name_and_user!(promo_code_name, user)
    if promo_code = PromoCode.from_name(promo_code_name)
      self.create_from_promo_code_and_user(promo_code)
    end
  end
.
.
.
end

Snippet 2:

  746 app/controllers/application_controller.rb
  250 app/helpers/indaba_url_helper.rb
  113 app/helpers/media_linkable_helper.rb
    5 app/helpers/facebook_helper.rb
  233 app/extensions/controllers/authenticated_system.rb
   59 app/extensions/controllers/auth_api.rb
   73 app/extensions/controllers/oauth.rb
   15 app/extensions/controllers/payment.rb
  128 app/extensions/controllers/page_caching.rb
   78 app/extensions/controllers/social_sharing.rb
 1700 total