Should I move my custom methods to model from controller?

Question

Let's say I have a Product model and ProductsController. Controller has all the standard CRUD method and Product does all kinds of validations etc.

Here is an issue. I have several custom very complex actions which also need to respond in multiple formats( json, html, xml, csv, pdf etc). Business logic reasons for this are beyond the scope of the question. Let's just way this is the way it has to be done. Also I use InheritedResources gem but I don't think it matters for the question.

For example ( this is a mock app, which is GREATLY simplified - I removed all kinds of if else statements and loops and localization, etc ):

class ProductController < InheritedResources::Base
  ....
    def check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order
      @order = Order.new
      @product = Product.find(params[:legacy_alphanumeric_product_number])
      if @product.stock > 5
        @po = LegacyOrder.create_po
        if @po
          if @order.save
            format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {success: "Wow! Input was good!"}}
            format.json{ render status: 400, json: {status: :success, message: "Order created"}}
          else
            format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {error: "Can't create order, some validations failed"}}
            format.json{ render status: 400, json: {status: :error, message: "Problem with order", errors: @order.errors}}
          end
        else
          format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {error: "Can't create order, PO number wasn't generated"}}
          format.json{ render status: 400, json: {status: :error, message: "Problem with po", errors: @po.errors}}
        end  
      else
        respond_to do |format|
          format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {error: "Can't create order, stock is low"}}
          format.json{ render status: 400, json: {status: :error, message: "Problem with product", errors: @product.errors}}
        end
      end  
    end   
  ....
end 

This is just to give an idea of complexity of some actions.

Now the question: should all of that goodness be moved into model? I'm dealing with business logic, which should be in the controller, but with trying keep with the rule of thumb Fat Models & Thin Controllers, it seems to me that it should be moved away, if so then what there is left to move?

Bonus Q: I'm coming accross use cases where I might need to use some of that functionality in the code, not through a REST interface. I.E. I need to use check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, while running a rake task. Like generating some orders based on low stock, or from an email event, etc. While I can use options described here: How do I call controller/view methods from the console in Rails? , having this action as part of a model would make it easier wouldn't it?

So what is a Rails Best Practices course of action in a case like this?

Answer 1

Consider moving your logic into a service object. I think shoving controller logic into the model is simply moving the problem to a different location. Yes, you do isolate the logic to a single area, but there are instances where you end up moving logic to models because of convention rather than the fact that it really belongs there.

A service object can help you reduce duplication and isolate your business logic without getting the model too involved in things it does not need to know about (your repeated json response, for example).

class OrderService
  def initialize(legacy_alphanumeric_product_number)
    # do stuff
  end
  # do more stuff
end

From the controller, you can just call

def check_whatever
  @order = OrderService.new(params[:some_product_code])
  @order.check_something
  # do more stuff
end

Take a look at 7 Patterns to Refactor Fat ActiveRecord Models. I found it very helpful. There is also a RailsCasts episode on service objects (requires pro subscription).

Answer 2

Take a look at the Draper gem (https://github.com/drapergem/draper). It provides nice decorator style wrappers that are the perfect place for putting logic on what to show or respond to. I agree with @mohamad that it doesn't belong in the model or controller. A service object is definitely a good way to go, or using Draper to create presentation specific logic methods.

Answer 3

As I can see, your problem is not moving the code to model from the controller, but re-factoring the controller method of

check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order

As an example

format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {success: "Wow! Input was good!"}}
format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: {error: "Can't create order, stock is low"}}

are same except their flash message. So you might probably generate the flash message only, like:

#conditions for the flash message
format.html{ render :check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order, flash: msg}

So try to re factor your code and personally I will not give the xml, json out put in the same controller. Because having a web api method like check_stock_using_legacy_identifier_and_create_a_unique_po_number_and_place_an_order does not look nice for me :)

I would move all my web api methods to a namespace like:

<url>/api/ etc

And if you have a good test coverage you can just go ahead and re-factor the code, if not now you know the advantage of having a good test suite :)