Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What is the best way to DRY this code up?

class NotificationsController < ApplicationController
  before_filter :load_user

  def unsubscribe
    if @facebook_admin && @facebook_admin.auth_key == params[:auth_key]
      @facebook_admin.update_notification_settings(params[:type] => false)
    elsif @user && @user.auth_key == params[:auth_key]
      @user.update_notification_settings(params[:type] => false)
    end
  end

  private

  def load_user
    if params[:type].present?
      if params[:facebook_admin_id].present?
        @facebook_admin = FacebookAdmin.find_by_id(params[:facebook_admin_id])
      elsif params[:user_id].present?
        @user = User.find_by_id(params[:user_id])
      end
    end
  end
end
share|improve this question
add comment

2 Answers

Verbosity of a controller is typically a signal of dubious design. As I see it, you have two sensible options:

  1. If User and FacebookAdmin are two very different user entities: they should be treated in two separate controllers.

  2. If User and FacebookAdmin are very similar (i.e. FacebookAdmin inherits from User): they can share the same controller but use always @user as variable. This way the code will be simplified dramatically.

What you should not have is a controller full of conditional branches, that will be an unmaintainable mess (and awful to test).

share|improve this answer
add comment
class NotificationsController < ApplicationController
  before_filter :load_user

  def unsubscribe
    [@facebook_admin, @user].each do |p|
      next unless p && p.auth_key == params[:auth_key]
      return p.update_notification_settings(params[:type] => false)
    end
  end

  private

  def load_user
    params[:type]
      .tap{|p| return unless p.present?}
    params[:facebook_admin_id]
      .tap{|p| return @facebook_admin = FacebookAdmin.find_by_id(p) if p.present?}
    params[:user_id]
      .tap{|p| return @user = User.find_by_id(p) if p.present?}
  end
end
share|improve this answer
1  
Thanks for your answer, sawa! Could you add a few words about what you changed and why? –  codesparkle Feb 14 '13 at 20:40
    
@sawa: Sorry, I don't see how a chain of imperative taps with inline conditions is better than good old conditional expressions. –  tokland Feb 15 '13 at 9:13
    
@tokland It avoids repetition of the method call Hash#[]. Instead of repeating params[:facebook_admin_id], you only need to write p. And the first tap in load_user is to make it symmetric with the other two taps so that it is easier for the eyes. –  sawa Feb 15 '13 at 9:16
    
ok, I see. An alternative to avoid this variable repetition is if (var = value). –  tokland Feb 15 '13 at 9:20
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.