Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
This is an extraction from Jim Weirich's "Decoupling from Rails" talk, which explained how to apply the hexagonal design pattern to make every layer of your application easily unit testable (without touching the database etc). It only seeks to extract a single method, the EmployeesController#create method, to illustrate the design damage that's …
# Original Rails controller and action
class EmployeesController < ApplicationController
def create
@employee = Employee.new(employee_params)
if @employee.save
redirect_to @employee, notice: "Employee #{@employee.name} created"
else
render :new
end
end
end
# Hexagon-inspired, test-induced, damaged version
class EmployeesController < ApplicationController
def create
CreateRunner.new(self, EmployeesRepository.new).run(params[:employee])
end
def create_succeeded(employee, message)
redirect_to employee, notice: message
end
def create_failed(employee)
@employee = employee
render :new
end
end
class CreateRunner
attr_reader :context, :repo
def initialize(context, repo)
@context = context
@repo = repo
end
def run(employee_attrs)
@employee = repo.new_employee(employee_attrs)
if repo.save_employee
context.create_succeeded(employee, "Employee #{employee.name} created")
else
context.create_failed(employee)
end
end
end
class EmployeesRepository
def new_employee(*args)
Biz::Employee.new(Employee.new(*args))
end
def save_employee(employee)
employee.save
end
end
require 'delegate'
module Biz
class Employee < SimpleDelegator
def self.wrap(employees)
employees.wrap { |e| new(e) }
end
def class
__getobj__.class
end
# Biz logic ... AR is only a data-access object
end
end
@JeroenDeDauw
Copy link

JeroenDeDauw commented May 15, 2014

In the video, Jim was exploring ways to decouple his domain code from Rails. It was not polished yet. At the beginning, he says: "What I wanna teach tonight is not 'this is how you do it' but 'here are techniques you can use to do it' ."

Exactly. This is a very important point. Yet most people will only be looking at the code snippet above. Which makes me think it is a bad idea to post the snippet like this if you want a proper discussion. To me this looks like it's set up for misinterpretation. Not cool.

@patmaddox
Copy link

patmaddox commented May 15, 2014

Here's an alternative solution that doesn't disturb DHH's original design: http://patmaddox.com/2014/05/15/poof-and-then-rails-was-gone/

@jurberg
Copy link

jurberg commented May 15, 2014

@JeroenDeDauw I agree. This reminds me of the comp.object newsgroup days. After posting how to implement an OO design for some problem using a simple example, there would always be someone who would write the simple example in a single function and try to use it as an argument against OOP. This is the same line of reasoning.

@bhserna
Copy link

bhserna commented May 15, 2014

I think that this example

# Original Rails controller and action
class EmployeesController < ApplicationController
  def create
    @employee = Employee.new(employee_params)

    if @employee.save
      redirect_to @employee, notice: "Employee #{@employee.name} created"
    else
      render :new
    end
  end
end

and this example

class CreateRunner
  attr_reader :context, :repo

  def initialize(context)
    @context = context
    @repo    = EmployeesRepository.new
  end

  def run(employee_attrs)
    @employee = repo.new_employee(employee_attrs)

    if repo.save_employee
      context.create_succeeded(employee, "Employee #{employee.name} created")
    else
      context.create_failed(employee)
    end
  end
end

are almost the same, the other classes are just there to integrate this code with rails.

I think I would prefer something like this.

module Employees
  class Actions::RegisterEmployee < Employees::Action
    def call
      employee = Employee.new(params.fetch(:employee))

      if employee.valid_for_registration?
        employee = store.create(:employee, employee)
        responder.success_response(employee)
      else
        responder.error_response(:unprocessable_entity, employee.regstration_errors)
      end
    end
  end
end

@joelmccracken
Copy link

joelmccracken commented May 15, 2014

If someone shows you a hello world example, it doesn't make sense to say "this example is invalid because I would never want to write a program that says hello world". It's just a very simple example.

@gmcinnes
Copy link

gmcinnes commented May 16, 2014

Props to @patmaddox there. That really is a beautiful answer. If you haven't looked at it, I encourage you to take a peek. I think it slices through a bit of Gordian knot here, and does it with specs :)

@solojavier
Copy link

solojavier commented May 16, 2014

This example is misleading. The talks is about how to modify your design when your application grows.

It's obvious that the original version is better than the modified in this case, but when the code evolves you may want to apply something like if there is a good reason to do it.

Design decisions will be guided by context...

Why don't we look at a controller like this to start with: http://www.slideshare.net/edbond/obie-fernandez-worst-rails-code/35 ?

@thefringeninja
Copy link

thefringeninja commented May 16, 2014

Like almost every code example, something is missing: context. It's not about 'I might use this in a console application,' it's that 'mysql is not working here, might need a KV store.'

@andyl
Copy link

andyl commented May 17, 2014

By far I prefer the @patmaddox version. Clean/concise/simple.

@depy
Copy link

depy commented May 21, 2014

To me it looks like this code with no proper context was put here with intention to mislead. To bad Uncle Bob is not on the "Is TDD dead?" talks.

@roysbailey
Copy link

roysbailey commented May 22, 2014

From the perspective of the "Is TDD dead?" debate, I am trying not to get too hung up on this specific code example. As other people have said - a "hello world" scale samples in any approach / framework are never comparable to real world situations. The point for me is, I have seen in many code bases in many companies which have similar characteristics - which are "large scale and reall world". In many cases, the multiple layers and abstractions seem to add no perceivable benefit to the solution. They simply pass data onto other components / layers (which inturn do the same) and so forth. This makes the code harder to maintain (ripple effect across components / layers) and in my experience add to the burden of trying to understand the code. I ask my self, why has this code been added? Is is people thinking they are following "best practice" for isolation purposes? Because it "turned out that way" from following a TDD approach. In my experience, the answer is varied.

For me, the key point is whether the "mockist" approach to TDD, which tends to lead to solutions where all / most behaviour is compartmentalised into individual abstractions (so we can mock them for testing purposes), leads us to better or worse designs.

There is a clear difference here for me between "classic TDD and mockist TDD". The former using state based verification and generally allowing tests with "real" collaborators, compared to the mockist approach, which in my experience tends to lead naturally to more abstractions (and behaviour verification).

In my opinion there is a complexity cost associated with each additional abstraction / layer added to a solution. Whilst the developer "owns the decision" about when to add new abstractions, it is clear in my mind at least, that TDD (in particular mockist TDD) does lead you (though not force you) into creating more. As developers I guess we need to take more responsibility as to when we add new abstractions / layers - and understand the tradeoffs associated. Is abstraction good? Well yes... but that does not mean you apply it verbatim to every single concept... I try to add new abstractions where I can see the benefit - and perform my "tests" slightly more "coarse grained"...

@kuzmik
Copy link

kuzmik commented Feb 24, 2015

Let's all be civil.

@rishabhp
Copy link

rishabhp commented Mar 9, 2015

Let's all be civil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment