Skip to content

@dhh /test_induced_design_damage.rb
Last active

Embed URL

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
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
@BJClark

Who actually writes applications like this?

@dhh
Owner

Please keep any discussion here civil. If you wish to present alternative solutions, make a blog post or a separate Gist, and link to it. But let's not turn this thread into the predictable mud bath these discussions so often end up in. Thank you.

@garybernhardt

BJ, I'm confused about that as well. Apparently Jim was advocating this publicly? (I didn't realize that.) I'd never write anything remotely resembling this; I'd write David's original version, exactly as he wrote it, byte for byte.

@thatrubylove

I had the pleasure of writing code like this with Jim before he died, although I argued heavily against the biz object crap and said they should just be pure ruby domain stuffs (modules and classes in lib).

I am surprised to hear Gary "rail" against this as test designed damage. I agree that it makes the controller less railsy, but my argument has been for 3 years now, that a restful controller is, 90% of the time broiler-plate that can be removed. I have done it many times by pushing it up the stack, that is where inherited_resources comes from, although I don't go that far and add model relation concepts in my controllers.

Anyway, I just wanted to chime in and say, that Jim never said "do this", Jim was exploring a "path" to a decoupled rails that was simple to test. He was looking for the unified theory of ruby. To my knowledge he paired with dozens of people like me on this last year, and possibly even in 2012.

We are sorely missing his voice now (at least I am), because I do like this adventure of looking for answers, even if I think a BizObject is stupid :)

@dhh
Owner

For more context, here's a description of the "hexagonal architecture" / "Ports & Adapters" pattern: http://alistair.cockburn.us/Hexagonal+architecture

And here's the blog post "Test-induced design damage" that explored the notion that applying such a pattern, and similar boundary-separation approaches, is actively harmful to the design of applications: http://david.heinemeierhansson.com/2014/test-induced-design-damage.html

@thatrubylove

To be clear, I like repositories, I despise runners :)

@neocsr

From watching the video, it seems that Jim's intention was to give us food for thought.
His main motivation was not testability, but framework decoupling.
The testing part was a side effect of that architectural change.

@crmckenzie

To answer the question of "who," that's pretty much exactly what I do in C# except when I'm scaffolding reference data. For most operations I'm doing a lot more than just CRUD. Most of our LOB operations involve some combination of multiple tables and/or databases, the cooperation of services, validation, security, and messaging. The business still just think of it as "do this thing." I don't think the CRUD examples illuminate why someone might choose this architecture.

Here's an example from a personal project I can share.


  [HttpPost]
  public virtual ActionResult AddCandidate(AddCandidateViewModel viewModel)
  {
       if (ModelState.IsValid)
       {
           var request = AutoMapper.Mapper.Map(viewModel);

           var response = this._positionService.AddCandidate(request);
           this.ModelState.Accept(response);

           if (this.ModelState.IsValid)
               return RedirectToAction("Candidates", new { id = response.PositionId });
       }

       var candidates = _candidateService.Query(null);
       viewModel.Candidates = new SelectList(candidates.Data, "CandidateId", "Name");
       return View(viewModel);
  }

UPDATE
I should add that I don't like the bidirectional dependency in the example. My view is that dependencies flow in one direction.

@BJClark

For the record, the only time TDD and/or Unit Testing appears on http://alistair.cockburn.us/Hexagonal+architecture is to link to someone else's blog post about it in the footnotes and in questions in the comments.

Hexagonal Architecture has little to nothing to do with TDD.

@phillmv

If it's not too forward of me, I would like to respectfully disagree re: inherited_resources. I would argue that it removes straightforward boilerplate in exchange for ratcheting up the complexity of modifying plain CRUD actions.

Maybe I need to set a variable for my views; maybe I want to filter on params; maybe this action's models need to be scoped to an association; and before you know it you've spent a lot of time dealing with the meta magic inherited resources provides and it's no longer clear, in my opinion, at a glance what is doing what and where.

This has been my experience, as I've spent a fair amount of my career rescuing projects and maintaining code other consultants have long since abandoned.

I think this is a relevant tangent because the same kind of fiery passion that leads to a preference for inherited_resources is related in kind to the same kind of passion that leads to complex test wrappers. I certainly commend the work and the effort.

Not all boilerplate is poisonous to the soul; sometimes the solution dramatically increases the conceptual overhead, and if I understand the testing debate correctly, we should have very well reasoned arguments for whenever we increase the complexity of our applications.

@ntl

Slight tangent, but I don't think you're going to see much benefit to a repository object when you're simply loading and storing data objects. It's irresponsible to introduce so much indirection to a use case like this that pretty much screams "CRUD."

But what if the use case isn't creating an Employee record, but instead awarding her a promotion? So, instead of an object called CreateRunner, I'm building an object called AwardPromotion. Suppose each promotion has a few, configurable requirements. As an example, let's say the following conditions need to be met for a particular employee to be awarded a promotion:

  • Either she needs a Master's degree -- her current position only requires a Bachelor's degree -- or her current supervisor needs to write a letter attesting to her qualifying work experience. That letter must have been signed by her supervisor's supervisor.

  • She needs to have completed an extra set of LMS courses (egads those things are the worst, but they are a real thing).

  • Her most recent performance evaluation must have been scored at at least a 3 out of 5.

  • Since the new position entails stock, she needs to have agreed to a few different forms for legal reasons.

Now I need to dynamically compose a promotion validation process based on the configuration state of the position the Employee is being promoted to. Those different criterion are totally unrelated to each other, and, again, totally dynamic/configurable. A promotion to a different position might entail completely different requirements. Also, many requirements may be inherited from the department you work in, others are configured by HR. I don't want to have to think about all that complexity when I'm writing AwardPromotion.

This is where TDD will shine, I can build up the complex AwardPromotion command object one use case at a time. My brain doesn't have to understand the entire system at once in order to build a working implementation. When I'm done, I can look at the actual patterns that have emerged and, with the help of my fast test suite, refactor it piece by piece into something that's easier to understand. In this way, TDD will be a powerful tool assisting the design of the code.

TL;DR if shoehorning all this indirection into typical CRUD use cases is actually common amongst rails programmers these days, and if they're only doing this to arrive at super fast tests, then I agree completely with @dhh that this is test induced design damage. Patterns like DCI and command objects should not be reached for solely for the purpose of speeding up tests. Simple solutions suffice for simple problems.

@sarudak

I don't like rails largely because of the active record pattern

But I don't particularly like the refactored result either. Since the dependency between createRunner and employeesController is bidirectional. And it's obviously over complicating a simple matter.

That said it's impossible to demonstrate why you would want a more complex architecture with such a simple example. Whereas it's nearly impossible to demonstrate how a more complex architecture might look with a sufficiently complex example to warrant a why and still keep people engaged.

@dalizard

@dhh I have this feeling that you are aware of the fact that this shown refactored version is highly exaggerated, and not what TDD would strive for.

@stravid

Jim said himself that he normally would not apply the pattern to this example because it is so simple. He specifically says that we should imagine that there is more stuff going on.

So why is this example picked again? To show something that Jim says himself? That this is a bad example?

I'm really disappointed that once again things are conveniently ignored to show of how TDD is bad.

Please watch the video. Jim specifically says that this is an oversimplified example and that you should not apply any decoupling patterns in this situation. He is only doing it to show how it is done.

@dalizard

@stravid Well said.

@swanson

This evented/callback style seems better suited for a desktop or mobile application. In those cases, there is a desire to keep the UI responsive to user input and I think decoupling with events (or "reactive" or "KVO" or whatever you are calling it) makes sense. In the context of a web application, where the main user interaction is an HTTP request, I think this amount of decoupling is confusing.

I reached the same conclusions as others after watching Jim's video: it was an interesting experiment, but I wasn't convinced that I would like working in such a system.

Maybe a more middle-of-the-road approach would be something like Command-Query Separation (CQS). The controllers would be simply delegating to Command or Query objects, which could be tested in isolation (if desired) - but still maintaining the procedural, top-to-bottom flow of execution. You could probably argue that AR models can play both the Command role (#save, #update) and Query role (#first, #where) without the need for additional abstraction.

@ernie

The problem with these sorts of simple examples is their simplicity. Any task simple enough to be discussed within a time limit that most of us would be willing to spend reading an article or watching a video makes the process being illustrated appear overwrought.

This, I think, is one reason why we have people who speak from hard-won experience about the benefits a larger inventory of objects with smaller, easier to understand, intention-revealing APIs can bring, but very few compelling examples in the form of blog posts, talks, and the like. The domain complexity these patterns help us cope with would necessitate that more time be spent explaining the domain than the approach being taken, itself.

Are there canonical examples of more complex workflows that are readily understood and provide a jumping off point to work from? I'm exhausted with todo apps, blogs, project managers, and the like. I really liked @ntl's example of promoting an employee. Promotions, payroll, etc, seem like a good start.

The irony is that the lack of attention span that makes our eyes glaze over when we read swaths of procedural code looking for the one thing we actually care about is the same thing that prevents us from tolerating long-form exposition of approaches to help us deal with the problem.

@jeremyf

Jim Weirich actually weighed in on this in his wyriki repository: https://github.com/jimweirich/wyriki/blob/master/app/controllers/pages_controller.rb

Below is a snippet of code from his project.

class PagesController < ApplicationController
  include PageRunners

  def show
    @wiki, @page = run(Show, params[:wiki_id], params[:id])
  end

  def show_named
    run(ShowNamed, named_params[:wiki], named_params[:page]) do |on|
      on.success { |wiki, page|
        @wiki = wiki
        @page = page
        render :show
      }
      on.page_not_found { |wiki_name|
        redirect_to new_named_page_path(wiki_name, named_params[:page])
      }
    end
  end
end

My understanding is that he was presenting guidance for when the design had grown beyond the simple case.

@MarkMenard

I feel this is extremely unfair of @dhh to use this example because the author is no longer able to respond. As @stravid said Jim did not advocate this style for simple scenarios.

Jim said a lot of things in that talk. Among them:

  • He advocated an incremental approach.
  • He specifically said he wouldn't use this approach for simple CRUD operations.

He used a simple use case so the architectural components would be more easily identifiable. Also, his primary goal was domain isolation from external dependencies. Fast tests were simply a by-product of that goal.

@dhh is there an example you could use from someone who could respond directly? This would enable a direct conversation rather then this weird side ways situation. Unfortunately Jim can no longer speak for himself.

Additionally Jim's https://github.com/jimweirich/wyriki repository has a more refined and evolved example with some significant improvements over the material from his Cincy Ruby talk.

@m-mujica

I think this blog post from Uncle Bob is also relevant to this discussion. http://blog.8thlight.com/uncle-bob/2014/05/01/Design-Damage.html

@unclebob

Jim made it clear, in his video, that he would not begin an application this way. Indeed, the whole point of the video was to take an application that had begun the traditional rails way, and refactor it towards an hexagonal architecture.

He also explained when and why you'd do this. His quote, at the end of the video was:

"The thing I want to stress is that: I don't think Rails is evil. I don't think it's a bad framework. I think that as applications grow what it gives you by default is not good for growth."

As applications grow, the coupling to "convenient" frameworks becomes less and less "convenient" and more and more costly. There comes a crossover point where continuing with the old pattern is greater than the cost of refactoring. In my experience that crossover point is fairly early on in the life of an application. YMMV.

@booch

I've been struggling with understanding how to apply the hexagonal architecture to Rails since I saw @unclebob's presentation at Midwest RubyConf in 2011. I don't think this is quite the right direction; I'm still searching.

This refactoring doesn't make much sense in any case. (Except maybe the repository pattern, but that's not the gist of this gist.) Getting rid of the direct reference to Employee doesn't really gain us much. We still have to worry about the details of the employee object in the redirect_to and render calls. So we still have a dependency in that direction, but we've added a dependency in the opposite direction with the callbacks.

But this doesn't have anything to do with TDD. We can (and my teams have) easily test the controller as originally without having to instantiate real Employee objects or hit the database. And although I'm a TDD proponent, I personally don't like to test Rails controllers. I write them as such a thin layer of boilerplate that I have enough confidence that they'll work just fine, and have user acceptance tests (with Capybara) to back me up in case I'm wrong.

So I'm afraid this is yet another strawman against TDD. Especially since TDD doesn't necessarily lead to the hexagonal architecture, and the hexagonal architecture is about decoupling, not testability.

@thatrubylove

"Maybe I need to set a variable for my views; maybe I want to filter on params; maybe this action's models need to be scoped to an association; and before you know it you've spent a lot of time dealing with the meta magic inherited resources provides and it's no longer clear, in my opinion, at a glance what is doing what and where."

This is YOU fighting RAILS, stop it. Let Rails have it's opinions here.

@thatrubylove

"I feel this is extremely unfair of @dhh to use this example because the author is no longer able to respond. As @stravid said Jim did not advocate this style for simple scenarios."

I do believe it was Gary that started it, and it wasnt clear to him this was someone cargo culting Jim doing "playtime for the mind" as he called it (at least in my session).

I didnt read the rest as you apparently didnt read the first :)

@thatrubylove

EVERYONE STOP!

No one (with any amount of experience in Rails dev) is saying you should do this, or that this is hexagonal. Hexagonal is bout YOUR DOMAIN.

So you have an app that is about a local car marketplace. Now do this mental exercise:

Make space in your mind for 2 lists.

In the first, put everything that the FRAMEWORK deals with... hint.... HTTP, sessions, persistence, validation, html, css, etc

Now make a second list, that list is simply EVERYTHING that isn't in the FIRST list that has to do directly with your DOMAIN.

Now your DOMAIN MODEL will benefit from hexagonal architecture as driven by TDD. Also, any SERVICE OBJECTS you can extract out of Rails can as well.

But if you are talking about MVC, and you try to change MVC and not the domain. You are doing Test Damaged Design.

This went from a straw man, to a burning straw man, with hexagonal being burned at the stake and Jim being misrepresented.

TDD IS AWESOME, HEXAGONAL DESIGN IS AWESOME

Mutilating Rails is not. If you want some shit like the above, do what DHH did, do what Gary did and write a framework where that was meant to happen. :D

@garybernhardt

if you start a comment like that you gotta finish it with "hammer time" is all I'm trying to say

@alexandreaquiles

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' ."

@mbriggs

It all depends on what type of application you are writing. If that is the sum total of your controller action, obviously it is over design. Jim was explicitly giving a talk about what to do when you dont have trivial amounts of coordination that needs to happen. The "Rails Way" works great, until the point that it doesn't. At which point, you take the tools Jim was trying to give you, and build something more elaborate to handle the additional complexity.

When you are talking about solving problems of complexity, you are in a difficult position. By definition, you are trying to impart a solution to a problem that is large and nasty, and could easily take a full hour to explain to the audience. So what do you do? Either have a 2 hour talk, or try to show the pattern in a way that can be understood without requiring huge amounts of background knowledge about the domain of the example.

Jim tried to explain this a few times in the talk, I am assuming @dhh must have missed it.

To put it another way, let's say the point of your application is to output "hello world". Rails architecture is MASSIVE overkill for that problem. Does that mean that rails is over engineered? Of course not. It is as complex as it needs to be to solve the type of problems it was built to solve. Presenting that talk in this light is making a very similar sort of claim.

@JeroenDeDauw

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

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

@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

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

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

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

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

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

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

@depy

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

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

Let's all be civil.

@rishabhp

Let's all be civil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.