How can I make this code DRY in my controller?

What is the best way to DRY this code up?

class NotificationsController < ApplicationController

def unsubscribe
elsif @user && @user.auth_key == params[:auth_key]
end
end

private

if params[:type].present?
elsif params[:user_id].present?
@user = User.find_by_id(params[:user_id])
end
end
end
end

-

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

def unsubscribe
next unless p && p.auth_key == params[:auth_key]
end
end

private

params[:type]
.tap{|p| return unless p.present?}

@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