Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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)


  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])
share|improve this question

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

Your Answer


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.