Rails refactor code

Question

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.

My Question is: in Rails MiniTest: I want to test a controller that get user_phone param value from HTTP X Header. how can I sent this param in my video_controller_test.rb file. like in my video_controller.rb get values as: user_phone = request.env['HTTP_X_MSISDN'].to_s — Kaleem Ullah
– Kaleem Ullah, Commented Dec 11, 2015 at 9:30

Pedro Nascimento · Accepted Answer · 2013-07-30 13:54:31Z

5

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

answered Jul 30, 2013 at 13:54

Pedro Nascimento

14k4 gold badges38 silver badges64 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Salil · Accepted Answer · 2013-07-30 07:49:41Z

1

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

answered Jul 30, 2013 at 7:49

Salil

47.7k22 gold badges127 silver badges161 bronze badges

Comments

jurglic · Accepted Answer · 2013-07-30 13:42:36Z

-1

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.

answered Jul 30, 2013 at 13:42

jurglic

3,7551 gold badge20 silver badges14 bronze badges

1 Comment

Pedro Nascimento Over a year ago

If you just change find_by_id to find, an error will occur. Check out my answer on how to handle it.

Collectives™ on Stack Overflow

Rails refactor code

3 Answers 3

Comments

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related