A block of stone possess certain characteristics, blemishes, and fissures. It is the sculptors job to address these as they arise. The sculptor must engage the stone in a dialectical conversation. As developers we must have the same conversation with our code. Let's channel our inner Michelangelo and head down this path together.

We've been asked to create a way for a user to enter orders into the system.

class Order < ApplicationRecord
end

class OrdersController < ApplicationController
def create
    @order = Order.new(params[:order])
    if @order.save
      redirect_to @order
    else
      render "new"
    end
  end
end

And our work is done for now and we can eat a bowl of soup. But by afternoon, we've learned that when an order is saved it also must notify watchers, generate documents, and notify the accounting system. Before we get started though, let's add some tests.

RSpec.describe OrdersController, type: :request do
  before do
    sign_in_user
    @order = spy(Order)
    allow(Order).to receive(:new).and_return(@order)
    post orders_path, params: { order: order_params }
  end 
  
  it 'notifies watchers'
    expect(@order).to have_received(:notifies_watchers)
  end
  
  it 'generates documents'
    expect(@order).to have_received(:generates_documents)
  end
  
  it 'notifies_accounting'
    expect(@order).to have_received(:generates_documents)
  end
end

These test seems very straight forward and they are. However, these tests are now the basis for all future development because they are written in a way that means that if they need to be modified we should consider it a code smell. Why is that? The Query vs Command testing pattern. What we've setup here is a series of commands and if we ever have to modify these tests we've switched from the command pattern to the query pattern which suggests we're putting domain concepts in our controllers. Let's pick up our chisel and get to carving.

class Order < ApplicationRecord

  def notify_watchers
    ...
  end
  
  def generate_documents
    ...
  end
  
  def notify_accounting
    ...
  end
end

class OrdersController < ApplicationController
  def create
    @order = Order.new(params[:order])
    if @order.save
      @order.notify_watchers
      @order.generate_documents
      @order.notify_accounting
      redirect_to @order
    else
      render "new"
    end
  end
end

In the pull request a fellow team member has a request. They'd like to be able to create orders via an API. We look at the code base and get started and here's what we come up with.

RSpec.describe Api::V1::OrdersController, type: :request do
  before do
    sign_in_user
    @order = spy(Order)
    allow(Order).to receive(:new).and_return(@order)
    post api_v1_orders_path, params: { order: order_params }
  end 
  
  it 'notifies watchers'
    expect(@order).to have_received(:notifies_watchers)
  end
  
  it 'generates documents'
    expect(@order).to have_received(:generates_documents)
  end
  
  it 'notifies_accounting'
    expect(@order).to have_received(:generates_documents)
  end
 end
  
class Api::V1::OrdersController < ApiController
  def create
    @order = Order.new(params[:order])
    if @order.save
      @order.notify_watchers
      @order.generate_documents
      @order.notify_accounting
      render json: { status: 'success' }
    else
      render json: { status: 'failure', errors: agency.errors.messages }, status: :bad_request
    end
  end
end

This doesn't feel great. We've now violated DRY(Do Not Repeat Yourself). It isn't the code duplication that is the problem though. Code can be duplicated and still not violate the DRY principle, "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". Let's do something about that by creating a service object.

class SuccessfulOrderProcessor

  def initialize(order)
    @order = order
  end
  
  def execute
    @order.notify_watchers
    @order.generate_documents
    @order.notify_accounting
  end
end

class OrdersController < ApplicationController
def create
    @order = Order.new(params[:order])
    if @order.save
      SuccessfulOrderProcessor.new(@order).execute
      redirect_to @order
    else
      render "new"
    end
  end
end

class Api::V1::OrdersController < ApiController
def create
    @order = Order.new(params[:order])
    if @order.save
      SuccessfulOrderProcessor.new(@order).execute
      render json: { status: 'success' }
    else
      render json: { status: 'failure', errors: agency.errors.messages }, status: :bad_request
    end
  end
end

We could even eliminate the service object and replace it with callbacks on the object after save. But in either case, if we revisit the dry principle, we haven't fully created a system in which "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". Looking at the service object, we've created a single location for the process of what to do when you have a successful order, but we're still faced with the problem of what is the single location where an order can be initiated. Let's see if we can fix that.

class OrdersController < ApplicationController
  def create
    @order = Order.new(params[:order])
    respond_to do |format|
      if @order.save
        SuccessfulOrderProcessor.new(@order).execute
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
end

We've now completely eliminated the need for the service object. So let's move back to the controller as service object concept. We've done that by removing the service object and replacing it with explicit calls to the various methods. And as a reminder, this whole time, our tests have been green because they've been constructed properly to allow for a refactor in this way.

class OrdersController < ApplicationController
  def create
    @order = Order.new(params[:order])
    respond_to do |format|
      if @order.save
         @order.notify_watchers
         @order.generate_documents
         @order.notify_accounting
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
end

We have no David on our hands yet. And it turns out that one of our coworkers just found out that we shouldn't be generating documents for agencies that self issue in their office. And so we make the change.

class OrdersController < ApplicationController
  def create
    @order = Order.new(params[:order])
    respond_to do |format|
      if @order.save
         @order.notify_watchers
         @order.generate_documents unless @order.agency.self_issue?
         @order.notify_accounting
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
end

And because we forgot to TDD, we go to update our tests and realize our commitment earlier to our tests. There has to be a better way to do this. And of course there is.

class Order < ApplicationRecord
  belongs_to :agency

  def notify_watchers
    ...
  end
  
  def generate_documents
    return unless agency.self_issue?
    ...
  end
  
  def notify_accounting
    ...
  end
end

class OrdersController < ApplicationController
  def create
    @order = Order.new(params[:order])
    respond_to do |format|
      if @order.save
         @order.notify_watchers
         @order.generate_documents
         @order.notify_accounting
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
end

Admittedly, this example is very simple and hopefully we'd all add the logic to the model, but as the complexity grows and relies on queries (vs command) methods earlier in the controller we see business logic seep out and into the controller. But we've got another request. It's obvious now that the API and the normal controller path have different requirements. We might be tempted to do something like the following.

class Order < ApplicationRecord
  attribute :api_request
  
  belongs_to :agency

  def notify_watchers
    ...
  end
  
  def generate_documents
    return unless agency.self_issue?
    ...
  end
  
  def notify_accounting
    ...
  end
end

class OrdersController < ApplicationController
  def create
    respond_to do |format|
      if @order.save
         @order.notify_watchers unless @order.api_request
         @order.generate_documents unless @order.api_request
         @order.notify_accounting unless @order.api_request
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
  
  private
  
  def new_order
    @order = Order.new(params[:order])
    @order.api_request = true if request.format.json?
  end
end

The shape of the code is telling us something. And that is that we have a domain concept here. We've already brought our controllers together and we don't want to split them up, but we've added 4 conditionals to the controller. Also, we've seen this pattern before. Where we added a conditional before, we could easily extract it to the model. But I wonder if there might be another and more OOP way to do this.

Before we do that though, we run our tests. They've turned red on us. And we wrote our tests in a certain way so that they would serve as an indicator when we were moving in a direction that might be less useful for us in the future. When we look back at the the active record object Order we see that the methods that exist on it don't have anything to do with active record at all. We extract those methods to the appropriate decorator and have freed our activerecord object of additional responsibilities.

class Order < ApplicationRecord
  belongs_to :agency
end

class OrderDecorator < SimpleDelegator
  def notify_watchers
    ...
  end
  
  def generate_documents
    return unless agency.self_issue?
    ...
  end
  
  def notify_accounting
    ...
  end
end

class ApiOrderDecorator < SimpleDelegator
  def notify_watchers
    true
  end
  
  def generate_documents
    true
  end
  
  def notify_accounting
   true
  end
end

class OrdersController < ApplicationController
  def create
    respond_to do |format|
      if @order.save
         @order.notify_watchers
         @order.generate_documents
         @order.notify_accounting
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
  
  private
  
  def build_order
    if request.format.json? 
      @order = ApiOrderDecorator.new(Order.new(params[:order]))
    else
      @order = OrderDecorator.new(Order.new(params[:order]))
    end
  end
end

And this works. The build_order method becomes a factory. We've reduced the number of conditionals that are needed from 4 to 1. This has broken apart the code in a way that makes sense and takes advantage of OOP. However, I think there are two refactors that are useful. The first one is renaming. We've named the decorators after their implimentation details. When we do this we create rigidity around their use and ability for other developers to reason about if they are appropriate for modification.

class Order < ApplicationRecord
  belongs_to :agency
end

class InternalInitiatingOrderDecorator < SimpleDelegator
  def notify_watchers
    ...
  end
  
  def generate_documents
    return unless agency.self_issue?
    ...
  end
  
  def notify_accounting
    ...
  end
end

class ExternalInitiatingOrderDecorator < SimpleDelegator
  def notify_watchers
    true
  end
  
  def generate_documents
    true
  end
  
  def notify_accounting
   true
  end
end

class OrdersController < ApplicationController
  def create
    respond_to do |format|
      if @order.save
         @order.notify_watchers
         @order.generate_documents
         @order.notify_accounting
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
  
  private
  
  def build_order
    if request.format.json? 
      @order = ApiOrderDecorator.new(Order.new(params[:order]))
    else
      @order = OrderDecorator.new(Order.new(params[:order]))
    end
  end
end

And the second one brings us full circle.

class Order < ApplicationRecord
  belongs_to :agency
end

class InternalInitiatingOrderDecorator < SimpleDelegator
  def save
    if __getobj__.save
      notify_watchers
      generate_documents
      notify_accounting
    end
  end
  
  def notify_watchers
    ...
  end
  
  def generate_documents
    return unless agency.self_issue?
    ...
  end
  
  def notify_accounting
    ...
  end
end

class ExternalInitiatingOrderDecorator < SimpleDelegator
  ...
end

class OrdersController < ApplicationController
  def create
    respond_to do |format|
      if @order.save
        format.html { redirect_to @order, notice: 'Post was successfully created.' }
        format.json { render :show, status: :created, location: @order }
      else
        format.html { render :new }
        format.json { render json: @order.errors, status: :unprocessable_entity }
      end
    end
  end
  
  private
  
  def build_order
    if request.format.json? 
      @order = ExternalInitiatingOrderDecorator.new(Order.new(params[:order]))
    else
      @order = InternalInitiatingOrderDecorator.new(Order.new(params[:order]))
    end
  end
end

With the exception of the build_order method being slightly changed, we're back to almost a completely vanilla Rails controller. We've also given back Order it's single responibility as an active record object.