# How can I make this code DRY in my controller?

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

-

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

-
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

-
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