0

I have a Log model that belongs to User and Firm. For setting this I have this code in the logs_controller's create action.

 def create
     @log = Log.new(params[:log])
     @log.user = current_user
     @log.firm = current_firm
     @log.save
   end

current_user and current_firm are helper methods from the application_helper.rb

While this works it makes the controller fat. How can I move this to the model?

3
  • Personally I don't think what you have here is fat, there might be good reasons to code it exactly as you have. You might want this in fact, to avoid putting user and firm in the attr_accessible for the model. Commented Dec 18, 2012 at 0:37
  • It's like this in too many places. Initially I was fine with it, but now it to much ;) Commented Dec 18, 2012 at 0:39
  • I agree about attr_accessible. One reason I like this in a worker class is it helps enforce a single point of entry for creating model instances, as well as enforcing that the necessary relations will be setup (though validation will also help with this; the worker method's signature helps act as a guide). It also makes testing the functionality that will be found in the controller for manipulating models dead simple. Commented Dec 18, 2012 at 0:40

3 Answers 3

3

I believe this sort of functionality belongs in a 'worker' class in lib/. My action method might look like

def create
  @log = LogWorker.create(params[:log], current_user, current_firm)
end

And then I'd have a module in lib/log_worker.rb like

module LogWorker
  extend self

  def create(params, user, firm)
    log      = Log.new(params)
    log.user = user
    log.firm = firm

    log.save
  end
end

This is a simplified example; I typically namespace everything, so my method might actually be in MyApp::Log::Manager.create(...)

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

1 Comment

I really like this approach. I felt it was a bit awkward in the model and in the controller. Thank you!
0

No difference: You can refactor the code:

def create
  @log = Log.new(params[:log].merge(:user => current_user, :firm => current_firm)
  @log.save
end

And your Log have to:

attr_accessible :user, :firm

Comments

0

Not much shorter, but the responsibility for the handling of current_user falls to the controller in MVC

def create
 @log = Log.create(params[:log].merge(
   :user => current_user,
   :firm => current_firm))
end

EDIT

If you don't mind violating MVC a bit, here's a way to do it:

# application_controller.rb
before_filter :set_current
def set_current
  User.current = current_user
  Firm.current = current_firm
end

# app/models/user.rb
cattr_accessor :current

# app/models/firm.rb
cattr_accessor :current

# app/models/log.rb
before_save :set_current
def set_current
  self.firm = Firm.current
  self.user = User.current
end

# app/controllers/log_controller.rb
def create
  @log = Log.create(params[:log])
end

2 Comments

Thank you for answering. The problem was a bit simplified in the question. I have more complex things going on. I just want to get it out. ;)
@Lyngstad I updated with a clean, but non-MVC way to handle that may be interesting to you if this kind of code is all over the place

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.