CODEWITHSUNDEEP
ruby-on-railsrubydry
krunal shah
krunal shah

Reputation: 16339

How can I dry this method?

  def restore_download_delete_file
    begin
      case params[:submit]
      when "restore"
        restore_status = restore_file(params[:file_names])
        raise if restore_status != 0
        flash[:notice] = "File Successfully Restored."
        redirect_to :action => "database_settings"
      when "download"
        download_status = download_file(params[:file_names])
        raise if download_status != 0
      when "delete"
        delete_status = delete_file(params[:file_names])
        raise if delete_status != 0
        flash[:notice] = "File Successfully Deleted."
        redirect_to :action => "database_settings"
      end
    rescue Exception => e
      flash[:error] = "Error with #{params[:submit]}! Please retry."
      redirect_to :action => "database_settings"
    end
  end

How could I improve this method?

Upvotes: 0

Views: 188

Answers (4)

mu is too short
mu is too short

Reputation: 434665

You can always turn your mega-method into a little interpreter:

private
FILE_CMDS = {
    'delete' => {
        :action   => lambda { |names| delete_file(names) },
        :notice   => 'File Successfully Deleted.',
        :redirect => 'database_settings',
    },
    'download' => {
        :action => lambda { |names| download_file(names) },
    },
    'restore' => {
        :action   => lamba { |names| restore_file(names) },
        :notice   => 'File Successfully Restored.',
        :redirect => 'database_settings',
    },
}

public
def restore_download_delete_file
    begin
        cmd    = FILE_CMDS[params[:submit]]
        status = cmd[:action].call(params[:file_names])
        raise if(status != 0)
        flash[:notice] = cmd[:notice] if(cmd[:notice])
        redirect_to :action => cmd[:redirect] if(cmd[:redirect])
    rescue Exception => e
        flash[:error] = "Error with #{params[:submit]}! Please retry."
        redirect_to :action => "database_settings"
    end
end

But I think egarcia's approach makes the most sense; there's really no need to mash all this stuff into one method. The commonalities really are quite minimal so four methods in one controller makes more sense: one action, one method. DRY is just a guideline, it isn't dogma to be followed at all costs.

Upvotes: 0

sawa
sawa

Reputation: 168101

 def restore_download_delete_file
    submit = params[:submit]
    if not restore_file(params[:file_names]).zero?
      flash[:error] = "Error with #{submit}! Please retry."
    elsif submit != "download"
      flash[:notice] = "File Successfully #{submit.capitalize}d."
    else
      return
    end
    redirect_to :action => "database_settings"
 end

Upvotes: 0

kikito
kikito

Reputation: 52651

You can clean it up by dividing it into four: one for restore, one for delete, one for download, and one for calling the appropiate one and handling the exceptions.

def restore_download_delete_file
  begin
    self.send "#{params[:submit]}"
  rescue Exception => e
    flash[:error] = "Error with #{params[:submit]}! Please retry."
    redirect_to :action => "database_settings"
  end
end

def restore
  restore_status = restore_file(params[:file_names])
  raise if restore_status != 0
  flash[:notice] = "File Successfully Restored."
  redirect_to :action => "database_settings"
end

def download
  download_status = download_file(params[:file_names])
  raise if download_status != 0
end

def delete
  delete_status = delete_file(params[:file_names])
  raise if delete_status != 0
  flash[:notice] = "File Successfully Deleted."
  redirect_to :action => "database_settings"
end

Also, a couple considerations:

  • Raise proper exceptions, not nil.
  • Don't rescue all exceptions. Rescue the ones you are raising instead.

Upvotes: 4

Ashish
Ashish

Reputation: 5791

Try like this

begin
  status = send("#{params[:submit]}_file", params[:file_names])
  raise unless status == 0
  if params[:submit] == 'restore' || params[:submit] == 'delete'
    flash[:notice] = "File Successfully #{params[:submit].capitalize}d"
  end
rescue Exception => e
  flash[:error] = "Error with #{params[:submit]}! Please retry."
ensure
  redirect_to :action => "database_settings" unless params[:submit] == 'download'
end

Upvotes: 1

Related Questions