Skip to content

Instantly share code, notes, and snippets.

@dhh
Created March 3, 2014 20:17
Show Gist options
  • Star 39 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save dhh/9333694 to your computer and use it in GitHub Desktop.
Save dhh/9333694 to your computer and use it in GitHub Desktop.
class GroupersController < ApplicationController::Base
def create
@grouper = Grouper.new(leader: current_member)
if @grouper.save
confirm_grouper_via_emails(@grouper)
enqueue_bar_assignment(@grouper)
redirect_to home_path
else
# No reason to use the flash, that's only for cross-request persistence
render :new
end
end
private
def confirm_grouper_via_emails(grouper)
LeaderMailer.grouper_confirmed(member: grouper.leader.id).deliver
WingMailer.grouper_confirmed(wings: grouper.wings.pluck(:id)).deliver
AdminMailer.grouper_confirmed(grouper: grouper.admin.id).deliver
end
def enqueue_bar_assignment(grouper)
AssignBarForGrouper.enqueue(grouper.id)
end
end
class GroupersControllerTest < ActiveSupport::TestCase
def setup
sign_in :david # fixture name reference for current_member
end
test "successfully created a grouper" do
assert_emails 3 do
post :create
end
assert_response :redirect
Grouper.last.tap do |new_grouper|
assert_equal members(:david), new_grouper.leader
assert_equal new_grouper, SomeQueue.last.grouper
end
end
test "failed to create a grouper" do
assert_emails 0 do
post :create
end
assert_response :ok
end
end
@JoshCheek
Copy link

Point 1: Here, we have one controller method (meaning routing endpoint). But in a real codebase, there are many controller methods. Now, all their helper methods are mingling together down in that private section. Your example seems more cohesive, b/c it's all right here, but in a real code base, that private section could quickly become unwieldy due to violating the Interface Segregation Principle. The flow of control gets lost, the cohesiveness falls apart, methods not being used and no one realizes it, disparate jumps around the file to find the method being called, no bigger-picture comprehension when reading a method.

Point 2: In this case, the logic is really simple: You create a grouper, it sends some emails. But many apps have complex interactions that occur. In this case, it can become necessary to test them in tandem. If the business logic is coupled to the controller, this becomes prohibitively difficult, and painfully expensive (slow).

An example to make it more concrete: the service receives notices to schedule payments, and that the payment failed, and needs to maintain correct accounting in accordance with these events. If the logic for adding payments, updating billing state, declaring events to accounting, etc is all tied up in the controller, then how do you test something like "customer is billed for $100 on 03/01 and 04/01, on 04/02 the second payment fails, we expect $100 in paid, $100 in past-due, and $10 in service-fees, customer status is 'suspended', customer is notified" and TBH, even this example omits many of the real-world complexities.

Pulpit: The entry points into the system should just be wiring, because if you can't decouple from the controllers and background jobs, you lose the ability to interact with your app without having to account for the entire world. The controller should depend on your application, not the other way around. The controller is just an adapter to the web.

It surprises me that you don't see this, you said Basecamp uses this style. But in Rework, you admit "These days, we have more resources and people, but we still force constraints [...] And we always keep features to a minimum", so perhaps being coupled to Rails works for you because you keep the domain shallow... Though, I do remember now that you had to rewrite Basecamp, and finding that post, we see one of the reasons given: "Over time, software builds up legacy. The old technology is baked in, and the roots of the product are so knotted that simply unwinding them becomes a massive undertaking. Think about trying to uproot a 250-year-old oak tree versus a two-year-old one."

This example, if it were in the real application with all the other controller methods and all their private methods and all that business logic written in the same way, would be the kind of knotted up roots that are difficult to unwind.

...I'm cool with this example, though ;)

@micahlmartin
Copy link

@JoshCheek 👍

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