CODEWITHSUNDEEP
ruby-on-railscontrollerrefactoring
akbarbin
akbarbin

Reputation: 5105

Rails refactor code

I had a code below:

  @brand = Brand.find_by_id(params[:id])
  if @brand.nil?
    flash[:error] = Flash.record_not_found
    redirect_to admin_brands_path
  end

And another change to below:

@brand = Brand.find_by_id(params[:id])
return(flash[:error] = Flash.record_not_found and redirect_to admin_brands_path) if @brand.nil?

Which code do you think is more efficient and explain? and when you have another suggest you can share too.

Thanks in advance.

Upvotes: 1

Views: 97

Answers (3)

Pedro Nascimento
Pedro Nascimento

Reputation: 13926

I'd do it like this:

def action
  @brand = Brand.find(params[:id])
rescue ActiveRecord::RecordNotFound
  redirect_to admin_brands_path, flash: {error: Flash.record_not_found}
end

Upvotes: 5

jurglic
jurglic

Reputation: 3737

First option is definately much better - no doubt about it. It is readable, not too much logic inside and it does only what controllers are supposed to do. Having the least lines of code really isn't a good metric.

As far as refactoring goes, I would leave it just the way it is. Maybe change #find_by_id to #find, but that's it.

Upvotes: -1

Salil
Salil

Reputation: 47532

I feel the upper one code is better as it is easy to understand and very clean, however you can write it as following too

unless @brand = Brand.find_by_id(params[:id])
  flash[:error] = Flash.record_not_found
  redirect_to admin_brands_path
end

Upvotes: 1

Related Questions