A few people have asked for my opinions on DHH's recent video series. And others have bemoaned the lack of a critical take on the videos. So here's a critical take involving my opinions on DHH's 2nd video) in the series.
Why not start with Video #1 in the series? Because it was great! Code comments that explain why things are the way they are... those are incredibly useful to people new / unfamiliar with parts of a codebase. His points were well made and I find nothing worth commenting on. (Pun intended)
Video #2 though has the word "callbacks" in it and so I'm intrigued to hear what DHH has to say about it.
So here's some "real-time" opinions that I wrote down while watching this video for a third time. If you like this sort of thing, let me know in the comments below.
- Ew callbacks. I've been bitten enough times by these -- mainly cases where callbacks are happening when I do not expect them to. Things like where I might call
createon a model in a unit test and that causes some other behaviour via callback that isn't relevant to the test. I'd prefer to be explicit in these cases to save surprises.
- I've heard mention that these talks might include Rails concerns (modules being included to add behaviour to classes), so I'd suggest you stop right here, get out your favourite drink of choice and take a drink whenever you see a concern used in the Rails app.
1:35 - "Side effects"
- "Side effects .. has gotten also a bit of a bad reputation, especially in functional programming" -- Yeah, because random magical shit happening when you call a method isn't easily predictable. Having the code be explicit about what it's doing makes it easier to understand it now and later. The later part is what I think DHH is missing here.
2:13 - Messages Controller
@bucket.recordtakes far too many arguments. What is it doing with them all? And P.S. do you even newline your key / value pairs?
@bucket.record(new_message, parent: @parent_recording, status: status_param, subscribers: find_subscribers, category: find_category )
I think that this reads a little better -- and the Git diff would be neater if a new key was added and removed. It feels like
category might one be of those things that was added recently and just chucked onto the end here.
TBH a little surprised here that he's using
@parent_recording which is setup as a
find_category aren't. They're called explicitly here. I'd expect some sort of consistency... but maybe there's a reason for that? The methods are folded at the bottom of the controller so I can't get a good idea of what those are doing to really judge whether or not it's a good choice.
- This model is pretty neat. I cringe a little (PTSD, I guess) on the sight of the
- Interesting that the model doesn't inherit from
ApplicationModel. Probably a legacy app thing.
- Neat use of
after_commit :deliverirks me a little, but maybe it's due to me preferring to keep all the logic inside of methods? I'd write it like this:
def deliver return unless mentioner == mentioned ... end
DRINK -- there's a concern here. [bad joke about it being "concerning"]
- Okay, so this is using more callbacks. I feel like
remember_to_eavesdropcould be something set "further up" in the chain, probably in the controller. The controller itself could check to see if these things have changed, and then from there choose to send out the mentions.
The other thing is
Current -- used in
eavesdrop_for_mentions down the bottom of the code view here.
I already wrote about CurrentAttributes at length. Global variables magically being available everywhere in the application. Where is
Current.user set? How can I be sure it's set to a value here?
Abstracting out the logic for triggering the job
This whole eavesdropping thing feels like it could be wrapped in some other logic (in a controller, perhaps). It's practically begging for it. Basecamp probably doesn't want to be persisting Messages to the database in its tests and having this job code run every single time, but that's exactly what's going to happen here. It'd probably slow down the tests due to these side-effects.
It would be better abstracted out to a "service object" which creates the recording and then triggers this
EavesdroppingJob. Bonus thing there is that you can pass
current_user from the controller and hey look I just got rid of the
CurrentAttributes global variable thing.
An idea of what that might look like:
module Mention class EavesdropForMentions attr_reader :recording, :params, user def initialize(recording:, params:, user: ) @recording = recording @params = params @user = user end def run return unless eavesdropping? Mention::EavesdroppingJob.perform_later recording, mentioner: user end private def eavesdropping? (active_or_archived_recordable_changed? || draft_became_active?) && !Mention::Eavesdropper.suppressed? && recording.has_mentions? end def active_or_archived_recordable_changed? # code here to check change using recordable + params end def draft_became_active? # code here to check change using recordable + params end end
I don't have the big Basecamp app to play with, so i don't know for certain if this code will or won't work. What I do know is that it neatly encapsulates potentially performing the
Mention::EavesdroppingJob later and avoids the issue where saving a
Recording in any context might queue up a job as a side-effect. My approach here decouples those two things, allowing them to happen independently.
Essentially, it accomplishes the same thing in (probably) as many lines, but disconnecting it from the saving of the model is the big win in my mind.
Yay more global state appearing out of the blue (
Mention::Eavesdropper.suppressed?). What could possibly go wrong? How can I track down easily where this might be toggled in the codepath that leads to this method? This looks like it would make debugging hard.
DHH himself says (at ~11mins) he thinks that there might be situations where you don't want callbacks to happen. Okay, great. So make it so that it can be an optional part of your code (as above), rather than this spooky-action-at-a-distance
Re-organising the code to optionally trigger this eavesdropping behaviour would lead to a lower cognitive overhead for working with this code.
Special mention (ha) of
has_mentions? at the bottom of this module which does seem to at least abstract the behaviour of checking if something has mentions.
It feels a lot like
Current.set here is a cheap way of passing account through to
mention::Eavesdropper and its associated things. I am not sure why this is wrapped this way, given that the account would be accessible in the
Eavesdropper class -- assuming it's setup like this:
class Mention class Eavesdropper attr_reader :recording def initialize(recording) @recording = recording end ... end end
But at least, this
Eavesdropper class is abstracted away and isn't a concern. There feels to me like concerns are used as a bit of a "golden hammer" in this application.
13:45 - run-through of all the parts
DHH jumps straight from the controller to the
Recording::Mentions concern here. (DRINK)
My initial thought here was: how is someone unfamiliar with this application supposed to know that the
Recording::Mentions concern is where to look for this eavesdropping behaviour if they were to go about debugging it?
This is spooky-action-at-a-distance and it's the kind of code that I might've written a few years ago and felt very smart at the time, but then months later when I've gone back to visit it I've asked myself: "wtf was past-Ryan taking?"
DHH says around the 14:10 mark that "there's a fair amount of indirection here but it provides a very clear path of reading what's going on in the method". Out of anything else in this video, this is the #1 thing that I disagree with the most. The path is completely ambiguous to my "untrained" eye -- I am unfamiliar with this application.
It is "clever code" and that is dangerous because future-you will come back and look at the code in a few months time and wonder how it all fit together again.
And now we get to a class which actually has an idea of what the Single Responsibility Principle is.
(Interestingly, DHH can't navigate his own code at the ~14:30 mark)
I've already talked about CurrentAttributes before, so what I'll do here is just sigh longingly, wishing wistfully for the death of global state in any and all applications.
"Globals is not something that you should just litter over your application" -- MY MAN! This is what you're doing here. "Passing around this stuff isn't helpful". Ok, here's the #2 thing I disagree with. When passing things around, you gain an inkling for where the thing came from and if you followed the chain high enough you might find where it was originally defined. This
Current.person mumbojumbo hides all of that for no real good reason.
Not sure I can state my thoughts clearer than this: DEATH TO GLOBAL STATE.
Ok so that
Mention::Scanner approach looks pretty good. It's great that this code wasn't just thrown into
Eavesdropper because it was somewhat related. It's a separate concern, and moving that logic into
Mention::Scanner is a good approach.
I still feel strongly that this could just be a method call in the controller after the
@bucket.record is called.
ProjectCopier + suppression
The suppression chain underneath
suppress_events_and_deliveries wouldn't be necessary if this code previously opted-in explicitly to making these "events" and "deliveries". It still feels like a poor work around for something that could be tidied up with half an hour's worth of effort.
And at the ~20:10 mark, DHH can't find where the suppressible behaviour is brought in. This thing where the code is hard-to-navigate is a massive code smell. Again: if the "callbacks" were explicit rather than implicit, the suppression wouldn't be needed. The overall code footprint would be smaller, more explicit, and therefore easier to understand. The code does A, B, and C. No magic.
20:39 - Wrap Up
"I hope it's clear" -- it isn't. I've been doing this Rails stuff for 10 years now and if I saw this code in a codebase I would look into ways of making this more explicit to make it easier to work with.
As I've said previously: this code feels like "clever code". "Look at me using all these cool Ruby features! I am so smart!". Well, yeah. You are smart.
But then in several months time of this thing chugging along working perfectly, you'll encounter a bug, look at the code and wonder how the hell it all fit together. There is far too much magic here.
But I guess that's The Rails Way™.
20:46: "take all this logic and jam it into, what? The controller? A service object?" -- No. A transaction object (see my example back at 5:55) that created the recording AND explicitly triggered the mention scanner + deliveries.
Another approach would be to use dry-transaction. This gem provides a very neat DSL for setting up such a thing. I'd imagine it would go like this:
class CreateRecording step :create step :scan_for_mentions step :deliver_notifications def create(bucket: bucket, ...) # @bucket.record code goes here end def scan_for_mentions # MentionScanner goes here end def deliver_notifications # Delivery code goes here end
It'll all be wrapped up neatly in the one class. This is where it should go. Not a controller. Not a service object. But a transaction object, that clearly delineates the steps involved in the transaction. There's no magic here. The steps are run in the order they are specified in. And it's possible to abort the transaction at each step.
This is my preferred approach. Callbacks and their implicitness have caused so much harm in previous codebases that I would never reach for them again. Transaction objects with explicit orders of operations are what I will be doing instead.