Skip to content

Instantly share code, notes, and snippets.

@franckverrot
Created May 2, 2014 20:51
Show Gist options
  • Save franckverrot/364a0c4ff5ea97e29438 to your computer and use it in GitHub Desktop.
Save franckverrot/364a0c4ff5ea97e29438 to your computer and use it in GitHub Desktop.
Message-passing, layers and SRP
class InviteController < ApplicationController
# The controller's responsibility is to build an object graph
# that will do the hard work.
def accept
response = InvitationResponse.new(self)
scenario = InvitationScenario.new(response)
scenario.accept_invite_token_for_user(current_user, params[:token])
# The alternative is to implement both
#
# inviting_succeded([User])
# inviting_failed([Exception])
#
# and to replace the 4 lines above with this:
#
# scenario = InvitationScenario.new(self)
# user_repo = InviteRepository.new(scenario)
#
# scenario.accept_invite_token_for_user(params[:token], current_user)
end
end
# This class supplements "AcceptInvite". It will tell the data repository
# to find some data, and based on the outcome of the retrieval, will do some
# work and then tell its collaborator to act.
# This is where the business logic would go
class InvitationScenario
def initialize(collaborator, repository = InviteRepository.new(self))
@collaborator = collaborator
@repository = repository
end
def accept_invite_token_for_user(current_user, token_str)
@repository.find_user_by_token(current_user, token_str)
end
def user_found(user_record)
# Maybe send emails, write some audit logs, etc.
collaborator.inviting_succeeded(user_record)
end
def user_not_found(exception)
# Will tell the police
collaborator.inviting_failed(exception)
end
def find_user_failed(exception)
# Will tell your mom, something was really wrong
collaborator.inviting_failed(exception)
end
end
# This class tells its collaborator about the outcome of
# the data retrieval (it encapsulates ActiveRecord, which shouldn't
# leak in the controller)
class InviteRepository < Struct(:collaborator)
def find_user_by_token(current_user, token_str)
begin
user = current_user.invites.find_by_token!(token_str)
collaborator.user_found(user)
rescue ActiveRecord::RecordNotFound => exception
collaborator.user_not_found(exception)
rescue => e
collaborator.find_user_failed(exception)
end
end
end
# This class is a proxy to the Rails collaborator. It is used to avoid
# cluttering the Rails collaborator's code with too many methods.
class InvitationResponse < Struct(:controller)
def inviting_succeeded(user)
controller.redirect_to invite.item, notice: "Welcome!"
end
def inviting_failed(error)
controller.redirect_to '/', alert: "Oopsy!"
end
end
# The models looks like
class Invite < ActiveRecord::Base; end
class User < ActiveRecord::Base
has_many :invites
end
@franckverrot
Copy link
Author

Agreed, if the contract was badly designed or inappropriate I'd change:

user_found(user_record)

to

user_found_by_token(user_record, token)

but in this case, and re: temporal coupling, this is what makes the scenarios understandable. As you know that user_found method is called when the repository found a user, you also know instantly what will follow, which in this case is the consumption of the token.

I like to follow those method calls as they don't much things and it's easier to reason about/change/test over time.

Also, when I feel that cohesion is very low in my classes, I change the design to isolate the collaboration. The code would then be:

class InvitationScenario
  def accept_invite_token_for_user(current_user, token_str)
    @token_str = token_str
    @repository.find_user_by_token(current_user, @token_str)
  end

  def user_found(user_record)
    # collaborator here is the response object
    token_scenario = TokenComsuptionScenario.new(collaborator, user_record)
    InviteRepository.new(token_scenario).tap do |repo|
      repo.consume_token(@token_str)
    end
  end

  def user_not_found(exception)
    collaborator.inviting_failed(exception)
  end

  def find_user_failed(exception)
    # Send some errors to your monitoring system
    # Maybe tell the user we found they encounter an error
    # and you're working on a solution already
    collaborator.inviting_failed(exception)
  end
end

class TokenComsuptionScenario
  def initialize(collaborator, user_record)
    @collaborator = collaborator
    @user_record = user_record
  end

  # Could be promoted to a collaborator class
  # and this would require `consume_token` to
  # accept a collaborator as an argument
  def token_consumed(token_str)
    @collaborator.inviting_succeeded(@user_record)
  end

  def token_not_consumed(exception)
    @collaborator.inviting_failed(exception)
  end
end

@jsuchal
Copy link

jsuchal commented May 4, 2014

Oh, I see, so instead of changing the method to user_found_by_token you follow the convention of user_found and use the instance variables right? I was a bit confused why you did that, and not still convinced if this is better over being explicit.

However what raised my eyebrows is the hardcoded TokenConsuptionScenario and InviteRepository in user_found method. How would you test that and why did you do that in first place?

Also I would probably go with token_consumed(token) - since you probably can get user from token and remove the user_record from TokenConsumptionScenario or maybe pass it token_consumed(token, user)

The issue I have with this style ((and I was aiming at from the beginning) is you when you have a "imperative" sequence of actions like (find invite by token, mark the token as used, do some other stuff) it's get very verbose and complicated to construct such object graph with collaborators. Not to mention the readability.

I really do like the Tell don't Ask and the fact that you are pushing it to the extreme in a way that basically you don't need a return value. This AFAIK actually is the Actor Model for concurrency btw.

I love this code/ping pong and thanks for any responses so far.

@franckverrot
Copy link
Author

Re: object graph: this is why we need some sort of built-in DI in Ruby. Building the graph manually can't scale. Rubyists tend to think DI is a buzzword, or a nonsensical idea. And it is probably true they don't need it, as the architecture they use (Active Record), wouldn't benefit from any kind of DI. But once you're decoupling things, you can't work without proper DI.

This is why, while trying to prove a point elsewhere (and not TDDing my gists/doing that on top of my head on a Saturday morning :-D), I hardcoded some classes: this is the easy road everybody takes, but IMHO not the one that a medium-to-large apps requires.

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