Message-passing, layers and SRP

example.rb

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

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

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.

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.