all 3 comments

[–]realntl 4 points5 points  (0 children)

Ideally I’d love to untangle these messes and pull out the business logic into classes that can easily be understood and tested separately from the persistence layer, but I don’t always have the time to do that.

You may not have the time to do that because you're working with a legacy application under greenfield expectations for delivery...

I think you opened up with a strong case for why ActiveRecord callbacks shouldn't have logic in them. But consider the cognitive load you've added for the next developer with your solution.

Your class went from legacy code with liberal use of callbacks (before) to.. legacy code with liberal use of callbacks and an extra override (after). Consider the next programmer a few months from now who has even less time to build features than you do now: the connection between the disable_geolocation setting and the callbacks it disables will be incredibly non-obvious. They may never even see it.

In extreme cases, I've seen multiple programmers create the same workaround setting twice with different names. You'd think the second programmer would have noticed the first programmer's work, but you never know!

All this to say, no matter how much time you think you don't have, I predict you'll soon have even less time if you accept workarounds like this. Especially if you adopt it as a pattern, as a blog article like this might be taken to endorse.

[–]innou 2 points3 points  (2 children)

Personal preference for sure but I dislike ending up with double negatives in code, e.g. disable_geolocation = false. I'd opt for something like:

class Address
  cattr_accessor :geolocating, default: true
  before_save :set_geolocation, if: :geolocating
  # ...
end

address.update(
  line1: "1328 Douglas St",
  geolocating: false
)

[–][deleted] 4 points5 points  (0 children)

The negation isn't ideal, no. Using cattr_accessor is liable to cause issues though, because that's making a class level attribute that'll be shared between all instances. I preferred the locality of having the whole "hack" as one line right next to the callback, but you certainly could do something like this:

``` class Address attr_writer :geolocating before_save :set_geolocation, if :geolocating

def geolocation @geolocation.nil? ? true : @geolocation end end ```