Skip to content

Instantly share code, notes, and snippets.

What would you like to do?
class GroupersController < ApplicationController::Base
def create
@grouper = current_member)
redirect_to home_path
# No reason to use the flash, that's only for cross-request persistence
render :new
def confirm_grouper_via_emails(grouper)
WingMailer.grouper_confirmed(wings: grouper.wings.pluck(:id)).deliver
def enqueue_bar_assignment(grouper)
class GroupersControllerTest < ActiveSupport::TestCase
def setup
sign_in :david # fixture name reference for current_member
test "successfully created a grouper" do
assert_emails 3 do
post :create
assert_response :redirect
Grouper.last.tap do |new_grouper|
assert_equal members(:david), new_grouper.leader
assert_equal new_grouper, SomeQueue.last.grouper
test "failed to create a grouper" do
assert_emails 0 do
post :create
assert_response :ok
Copy link

JoshCheek commented Mar 6, 2014

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 ;)

Copy link

micahlmartin commented May 30, 2014

@JoshCheek 👍

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