How can I DRY this Rails code? [closed]

Question

users_controller.rb:

class UsersController < ApplicationController def new end def create user = User.create(params[:user]) if user.id session[:user_id] = user.id redirect_to '/city/map' else redirect_to new_session_path, flash: { error: 'This name is already taken.' } end end end

sessions_controller.rb:

class SessionsController < ApplicationController def new end def create user = User.new(params[:session]).authorize if user session[:user_id] = user.id redirect_to '/city/map' else redirect_to new_session_path, flash: { error: 'Your login information is invalid.' } end end def destroy end end

How can I DRY it (only in terms of repeatable code in create actions in these controllers)?

Mike Szyndel · Accepted Answer · 2013-07-14 10:49:59Z

Comments by Billy Chan are very valuable but I still think your question is worth response. So...

Is this code repetitive? Yes. Should you DRY it? No. Why?

To quote DRY principle from Wikipedia

"Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."

So what does it mean in general is that, you shouldn't repeat the same code twice, as this is a part of knowledge is a system (application).
But why do I think it is not the same, even if it's quite exactly same code in two controllers? Because the KNOWLEDGE hidden in those pieces of code is different. In each of those controllers you have logic that describes how to go around different pieces of data submitted by user concerning two different object classes. The behaviour of create action in controller is not universal enough that you may save all objects in the same manner. Sometimes you want to associate created object with current_user, other times there may be a need to add some tasks to background queue (posting to Facebook, compressing images). For this reason it would be unwise to make those methods one.

Billy Chan · Accepted Answer · 2013-07-13 12:39:42Z

Before drying, make sure your code working.

In the first block, your approach is wrong

user = User.create(params[:user])

But the user object is not persisted and no id will be assigned. (only create! will try to save to db and will throw error if failed)

A conventional approach is

user = User.new(params[:user]) if user.save # do something else # handling error end

In your second block, I don't know what will your custom method authorize do, and can't comment.

Actually User.create will never return nil, so it's not wrong but very unidiomatic, better + new + is x.save as you show. — tokland
– tokland, Commented Jul 13, 2013 at 12:11
@tokland, thanks for pointing out that. I mixed it with create! which will save to db. — Billy Chan
– Billy Chan, Commented Jul 13, 2013 at 12:40

Stack Exchange Network

How can I DRY this Rails code? [closed]

2 Answers 2

Hot Network Questions

How can I DRY this Rails code? [closed]

2 Answers 2

Related

Hot Network Questions