Skip to content

Instantly share code, notes, and snippets.

@dhh
Last active June 22, 2023 06:18
Show Gist options
  • Save dhh/4849a20d2ba89b34b201 to your computer and use it in GitHub Desktop.
Save dhh/4849a20d2ba89b34b201 to your computer and use it in GitHub Desktop.
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
@solojavier
Copy link

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

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

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.

Copy link

ghost 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