Skip to content

Instantly share code, notes, and snippets.

@gma
Created July 6, 2012 17:00
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save gma/3061327 to your computer and use it in GitHub Desktop.
Save gma/3061327 to your computer and use it in GitHub Desktop.
Moving response callbacks out of controller
class CardCreator < Struct.new(:listener)
def create(iteration, attributes)
card = iteration.cards.build(attributes)
if card.save
listener.created(card)
else
listener.create_failed(card)
end
end
end
class CardPersistenceResponder < Struct.new(:controller)
def created(card)
controller.instance_eval do
redirect_to planning_path(card.project)
end
end
def create_failed(card)
controller.instance_eval do
@title = 'New card'
@card = card
flash.now[:alert] = 'Your card needs a title'
render :action => 'new'
end
end
end
class CardsController < ApplicationController
def create
creator = CardCreator.new(CardPersistenceResponder.new(self))
creator.create(project.backlog, card_params)
end
end
@gma
Copy link
Author

gma commented Jul 6, 2012

The point of the CardPersistenceResponder is to move the callback methods that are called by the CardCreator off the controller (whose public methods are unfortunately "magic"). I'm not sure in my own mind how important that is. The controller's responsibility (acting as a RESTful interface to the web) is then a little clearer.

@gma
Copy link
Author

gma commented Jul 6, 2012

The CardPersistenceResponder would often have two more callbacks; updated and update_failed. I'm rather liking the separation of concerns in this approach. It might be a bit harder to test the responder though, given that it's executing blocks of code on the controller. You could argue that it's so simple it doesn't need unit testing, and just rely on a functional test or integration test to ensure that it works. The tricky shit that needs testing ought to be in the creator object.

@renato-zannon
Copy link

I like your solution, but changing instance variables non-locally bothers me a little... I've been using a callback-y approach to it, kinda inspired by javascript libraries:

class CardsController < ApplicationController
  def create
    creator = CardCreator.new(
      on_success: ->(card) { redirect_to planning_path(card.project) },
      on_error:   ->(card) { report_card_creation_failure(card) }
    )

    creator.create(project.backlog, card_params)
  end

private

  def report_card_creation_failure(card)
    @title = 'New card'
    @card = card
    flash.now[:alert] = 'Your card needs a title'
    render :action => 'new'
  end
end

I think that solves the 'magical methods' problem, while keeping the routing/rendering stuff inside the controller. What do you think?

@gma
Copy link
Author

gma commented Jul 9, 2012

@Riccieri – Nice use of the stabby lambda. It clearly bothered Steve Tooke a bit too; have a look at how he tweaked it:

https://gist.github.com/3062683

The SimpleDelegator is how I'm now doing it, and I've moved the responder class inside the controller (so it's just called PersistenceResponder). I prefer it because I'd rather use the observer pattern than explicit callbacks (it feels slightly more decopuled). I'll blog the finished result and link to it here…

@gma
Copy link
Author

gma commented Aug 10, 2012

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