current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

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)?

share|improve this question
add comment

2 Answers

up vote 3 down vote

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.

share|improve this answer
add comment
up vote 1 down vote

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.

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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.