Help me refactor this nasty Ruby if/else statement

Question

so I have this big method in my application for newsletter distribution. Method is for updating rayons and I need to assign a user to rayon. I have relation n:n through table colporteur_in_rayons which has attributes since_date and until_date.

I am a junior programmer and I know this code is pretty dummy :) I appreciate every suggestion.

def update
  rayon = Rayon.find(params[:id])
  if rayon.update_attributes(params[:rayon])
    if params[:user_id] != ""
      unless rayon.users.empty?
        unless rayon.users.last.id.eql?(params[:user_id])
          rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now)
          Rayon.assign_user(rayon.id,params[:user_id])
          flash[:success] = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}."
          return redirect_to rayons_path
        end
      else
         Rayon.assign_user(rayon.id,params[:user_id])
         flash[:success] = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}."
         return redirect_to rayons_path
      end
    end
    flash[:success] = "Rayon has been successfully updated."
    return redirect_to rayons_path
  else
    flash[:error] = "Rayon has not been updated."
    return redirect_to :back
  end
end

Answer 1

there was a some double code. So remove that. But also, there is some business code in the controller, which should not be there. So i should refactor the controller code as follows:

def update
  rayon = Rayon.find(params[:id])

  if rayon.update_attributes(params[:rayon])
    if params[:user_id] != ""
      rayon.handle_update_user(params[:user_id]
      flash[:success] = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}."
    else
      flash[:success] = "Rayon has been successfully updated."
    end
    return redirect_to rayons_path
  else
    flash[:error] = "Rayon has not been updated."
    return redirect_to :back
  end
end

The controller method now clearly deals with the actions that need to be taken, and sets a flash method accordingly if needed.

In the Rayon model you write the code what needs to be done when an update is done by a user:

class Rayon

  def handle_update_user(user_id)
    if (!users.empty? && users.last.id.eql?(params[:user_id]))
      # do nothing! 
    else 
      colporteur_in_rayons.last.update_attributes(:until_date => Time.now) unless users.empty?
      Rayon.assign_user(rayon.id,params[:user_id])
    end

  end

end

This clearly seperates the concerns. A rayon should know what happens when a user updates it (the name of the function could be improved to what you actually want it to mean, as that is not entirely clear to me). It could be shorter, but i like to write explicitely that nothing needs to be done if the last user is the same as the current. Otherwise, actions need to be taken. If i understood correctly.

Answer 2

def update
    rayon = Rayon.find(params[:id])

    unless rayon.update_attributes(params[:rayon])
        flash[:error] = "Rayon not updated."
        return redirect_to :back
    end

    puid = params[:user_id]
    empty = rayon.users.empty?

    if puid == "" or (not empty and rayon.users.last.id.eql?(puid))
        msg = "Rayon updated.",
    else
        msg = "Rayon #{rayon.name} assigned to #{rayon.actual_user.name}.",
        rayon.colporteur_in_rayons.last.update_attributes(
            :until_date => Time.now) unless empty
        Rayon.assign_user(rayon.id, puid)
    end

    flash[:success] = msg[msg_i]
    return redirect_to rayons_path
end

Answer 3

def update
  rayon = Rayon.find(params[:id])

  unless rayon.update_attributes(params[:rayon])
    flash[:error] = "Rayon has not been updated."
    return redirect_to :back
  end

  if params[:user_id].empty?
    msg = "Rayon has been successfully updated."
  else
    if rayon.users.empty?
      Rayon.assign_user(rayon.id,params[:user_id])
      msg = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}."
    elsif not rayon.users.last.id.eql?(params[:user_id])
      rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now)
      Rayon.assign_user(rayon.id,params[:user_id])
      msg = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}."
    end
  end
  flash[:success] = msg
  return redirect_to rayons_path
end

Answer 4

Here's my take at it. I put a bunch of comments inline describing why I changed what I did.

def update
    rayon = Rayon.find(params[:id])
    if rayon.update_attributes(params[:rayon])
        user_id = params[:user_id] # using same value a couple times
        # if-else strongly preferred over unless-else...
        # so replace `unless foo.empty?` with `if foo.length > 0`
        if user_id.length > 0 # then string != ""
            # `if A && B` more readable than `unless X || Y` in my opinion
            if rayon.users.length > 0 && rayon.users.last.id != user_id
                rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now)
            end 
            # same line in if-else; pull outside
            Rayon.assign_user(rayon.id, user_id)
            flash[:success] = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}."
        else
            flash[:success] = "Rayon has been successfully updated."
        end

        # all branches in here return this value, pull outside
        return redirect_to rayons_path
    else
        flash[:error] = "Rayon has not been updated."
        return redirect_to :back
    end
end

The largest issue I noticed with your code is that you had a lot of duplication in your first if-block. All the branches in the true section reached return redirect_to rayons_path, so that got pulled to the end of the block. You had the same flash[:success] = ... twice, so that got pulled out as well.

if-else is much more readable than unless-else, so try to avoid using the latter. I'm not a fan of one unless immediately nested inside another, so I changed that to a compound if-statement.

One of the primary idioms in Rails is DRY (Don't Repeat Yourself), so try to identify spots where you do have repetitive code.

_{(Also note that I haven't tested this refactoring -- I'm pretty sure that the logic is still the same, but I can't prove it.)}