How can I make this code DRY in my controller?

Question

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

Answer 1

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).

Answer 2

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