How do I refactor this block of code to make it dry

Question

How do I go about making this code block dry? I understand that dry means dont repeat yourself but I don't see any obvious opportunity for refactoring.

Index, show, edit, and create seem like basic/necessary methods. They appear to be pretty dry to me.

The methods after I am not sure about.

I haven't tried anything other than googling so far.

class UsersController < ApplicationController

  def index
    @users = User.all
  end

  def show
    @user = User.find(params[:id])
  end

  def new
    @user = User.new
  end

  def edit
    @user = User.find(params[:id])
  end

  def create
    @user = User.new(user_params)

    respond_to do |format|
      if @user.save
        format.html { redirect_to @user, notice: 'User was successfully created.' }
        format.json { render :show, status: :created, location: @user }
      else
        format.html { render :new }
        format.json { render json: @user.errors, status: :unprocessable_entity }
      end
    end

    Slack.notify_channel 
  end

  def update
    @user = User.find(params[:id])

    respond_to do |format|
      if @user.update(user_params)
        format.html { redirect_to @user, notice: 'User was successfully updated.' }
        format.json { render :show, status: :ok, location: @user }
      else
        format.html { render :edit }
        format.json { render json: @user.errors, status: :unprocessable_entity }
      end
    end

    Slack.notify_channel
  end

  def destroy
    @user = User.find(params[:id])

    @user.destroy
    respond_to do |format|
      format.html { redirect_to users_url, notice: 'User was successfully destroyed.' }
      format.json { head :no_content }
    end

    Slack.notify_channel
  end

  private
    def user_params
      params.require(:user).permit(:username, :email)
    end
end

There is no rails backend attached to this code snippet. I am assuming it is just theoretical - wanting us to refactor the code to make it shorter.

Answer 1

IMHO you can do something like that.

class UsersController < ApplicationController
  include ExceptionHandling
  before_action :load_user, only: [:show, :edit, :update, :destroy]
  after_action :slack_notify_channel, only: [:create, :update, :destroy]

  def index
    @users = User.all
  end

  def new
    @user = User.new
  end

  def create
    @user = User.create!(user_params)
    respond_to do |format|
      format.html { redirect_to @user, notice: 'User was successfully created.' }
      format.json { render :show, status: :created, location: @user }
    end
  end

  def update
    @user.update!(user_params)
    respond_to do |format|
      format.html { redirect_to @user, notice: 'User was successfully updated.' }
      format.json { render :show, status: :ok, location: @user }
    end
  end

  def destroy
    @user.destroy!
    respond_to do |format|
      format.html { redirect_to users_url, notice: 'User was successfully destroyed.' }
      format.json { head :no_content }
    end
  end

  private

  def load_user
    @user = User.find(params[:id])
  end

  def slack_notify_channel
    Slack.notify_channel
  end

  def user_params
    params.require(:user).permit(:username, :email)
  end
end

I would recommend you to create a concern to manage exceptions and render each specific error by exception. Then you can avoid having two ways in each action to render a good and bad case.

Answer 2

I have to guess a bit about what's in your ApplicationController and User classes. But the obviously repeated code is @user = User.find(params[:id]). Your show and edit methods both just run this line. So, they do exactly the same thing, which they oughtn't to do. One method for one thing.

Also, once you resolve show and edit into a single method, your create, update and destroy methods should call it instead of repeating the line.

Next, I wouldn't use new as a method name, since it is already used by BasicObject::new. I tested a few things and it gets pretty unclear:

class Test
  attr_reader :test
  def initialize
    @test = 'test'
  end
end

class Test2
  attr_reader :test2
  def new
    p 'test2new'
    @test2 = Test.new
  end
end

testx = Test2.new
p testx.new.test
p testx.test2.test

=> "test2new"
=> "test"
=> "test" 

It takes extra effort to see when you're calling your own new method and when you're calling BasicObject::new. So, I'd change that to new_user or something, if you even need it at all — I don't see why you need both it and your create method. (If you don't need it, get rid of it.)

Finally, @user = User.find(params[:id]) doesn't imply either showing or editing, so understanding what you're trying to do and coming up with a name that reflects it (e.g. set_user_id) is something that ought to be done as well.