When wrapping AR behind an intentional interface, go explicit or generic/idiomatic?

music_library.rb

# This is a record, as in a vinyl.
class Record < ActiveRecord::Base
  belongs_to :user
  belongs_to :album

  # explicitly stating what is needed
  def from_library_for_album(collector, album)
    where(user_id: collector).where(album_id: album)
  end

  # or, if we go more generic/idiomatic
  def from_library(collector, options={})
    album = options.fetch(:album) { false }
    status = options.fetch(:status) { :mint }

    records = where(user_id: collector)
    records = records.where(album_id: album) if album
    records = records.where(status: status) if status
    records
  end

end

class Album < ActiveRecord::Base
  has_many :records
  belongs_to :artist
end

class User < ActiveRecord::Base
  has_many :records
end

# I prefer to keep AR as a private interface used only w/in the Model.

class Library
  def initialize(collector, fetch_records=Record)
    @collector = collector
    @fetch_records = fetch_records
  end

  def fetch_records_for_album(album)
    fetch_records.from_library_for_album(collector, album)
    # ... or ...
    fetch_records.from_library(collector, album: album)

    # and what about more params?
    fetch_records.from_library_for_album_in_mint_condition(collector, album)
    # ... or ...
    fetch_records.from_library(collector, album: album, condition: :mint)
  end  
end

class LibrariesController < ApplicationController

  def album
    library = Library.new(current_user)

    @album = Album.find(params[:id])
    @records = library.fetch_records_for_album(@album)
  end
end

What do you think?

Initially I like the explicit method names, which force the AR Model to do nearly all of the work. However, that seems to get cumbersome quickly.

I like the from_library method, but fetch_records_for_album is too long. We know you're fetching, we know it's for an album, so why not just records_for.

@records = library.records_for(@album)

@steveklabnik, excellent point - the "fetch" bit does feel verbose. Thanks.

Been thinking about this a bit more and I'm trying to find an elegant way to handle multiple options needed for more complex queries. For example

class LibrariesController < ApplicationController

  def album
    library = Library.new(current_user)

    @album = Album.find(params[:id])
    @records = library.mint_condition_records_for(@album) # can can get harry w/more than a few "condtitions"

    # Or, we could have a more generic method, however that is a lot like the Record.from_library method, 
    #  and could result in an effectively huge interface, much like ActiveRecord's .find methods. What to do?
    @records = library.records_for(@album, :condition => :mint)
  end
end

It's verbose but not because of the length of the method names. That, to me, is more of an indication that your abstractions are leaking.

TL;DR;
ActiveRecord seems to be the thing you don't like. I recommend not using AR at all if you're taking this approach.

My thoughts:

From the outside, this sample indicates (and your comments make this somewhat explicit, so correct me if I'm missing something) but they indicate that you're actively trying to deny the class' identity that inheritance is dictating to your class. AR's relationship to the persistant store is public and you're using convention to try and make it private. Guarding against the surface area you've created here is going to be your primary concern and while you can mitigate it with convention it's going to feel like swimming against the current. You could patch AR to better define the abstraction you're looking to engineer but to me, the smell, the thing you're trying to solve here, is AR.

class Library
  def initialize(collector, fetch_records=Record)
    @collector = collector
    @fetch_records = fetch_records
  end
...
end

This initializer is where it all falls apart for me. Right here, I'm confused as to why you'd be pulling up internals of AR just to give the illusion that AR is a dummy repository with no public methods for hydrating objects. This is an odd assumption based on your class definition that will require care when validating and indoctrinating new members to your team. Are you trying to make Library a class that knows how to run queries on any AR record? Cause, I can send in a User class ref here and search on that, can't I? At that point, what did Library buy me? In the following example...

@album = Album.find(params[:id])
@records = library.fetch_records_for_album(@album) 
vs 
@album = Album.find(params[:id])
@records = @album.records

The library is introduced for no other reason but to search the album's relational data? But album, by your class definition, already provides this. What happens in your codebase when I replace it for the second implementation? Probably nothing but I'm guessing you're going to have a few interesting conversations regarding this approach if you're planning on doing this with others. Also, if @records doesn't need to be evaluated (conditional views, for example) you're performing a query eagerly for the sake of the interface.

Record, a class reference, in your Library example is referenced by a variable that reveals very little other than you want to deal with the persistence layer through a Repository instead of the class methods directly. Presumably to make lookups from Library instances polymorphic but if I'm being honest this all just seems like indirection wearing abstraction's clothing.

Your class for album declares a records association created by the ORM explicitly (via inheritance) in your object graph. Complex queries could easily be wrapped in named_scopes and would be far more idiomatic than the convention that is introduced here.

If the goal is testing, you can mock the database interaction a million ways. If the goal is a better API, I don't think this achieves your goal. It asks good questions but solves them with indirection and not abstraction. The library class is indirection for creating named_scopes in your models. If they can be reused (or should be, or whatever) then package the strategies into modules and include them in the classes where appropriate that way your API's surface area remains tight, you have a small surface area to test and you're not rearranging deck chairs on the Titanic just to get around the harsh reality that AR is establishing your object graph in ways that you don't agree with.

I'm inspired by the tenacity with which you're attacking this beast but I must admit that it reads less like "I have a brilliant idea" and more like "I really hate AR and want to hide it." In which case, I'd say... fuck AR. Fuck AR right in the ear and write the classes that you really want to use.

Perhaps I'm missing something so let's make sure to chat this in full detail over dangerous amounts of alcohol at CodeMash. :)

@leongersing
As always, I much appreciate the thoughtful feedback! Overall I agree that it feels like a lot of indirection for little benefit, and certainly a broken, if not entirely missing, abstraction.

Re: the Library - My intent was to use it to centralize the concept of exactly what Artists, Albums, and individual Records a given User (collector) has. That is, I wanted there to be single, and obvious way to query his/her library. The desire my just be a knee jerk, and inappropriate, reaction to seeing this kind of querying sprinkled, and repeated, all over a code base. Take for instance the point you raised about going directly to the Album instance to fetch it's records:

@album = Album.find(params[:id])
@records = @album.records

That isn't exactly what we're after; that would return ALL instances of that album, across all collectors. Instead, we're after something a bit more complicated - @album.records.collected_by(@current_user). If we roll with that, the domain concept of the actual records user has in his library is muddied; there is no one source of the truth. Does that make sense?

In that vein, you mentioned named_scopes may be a cleaner and more idiomatic way to attack this issue. You're probably right and in looking at my real domain (which looks like the following), I think adding some more named scopes, and judiciously combining them across models via Relation#merge would go along way.

UPDATE: I'm using a bit of my real domain to help illustrate, so Beer is a moral equivalent of Record from the example above.

class Beer < ActiveRecord::Base
  belongs_to :brew
  belongs_to :user

  validates :brew, presence: true
  validates :user, presence: true
  validates :status, inclusion: [:stocked, :drunk, :traded, :skunked]

  attr_accessible :batch, :bottled_on, :best_by

  scope :stocked, where(status: :stocked)
  # etc...

  def self.from_cellar(keeper, options={})
    brew = options.fetch(:brew) { false }

    beers = stocked.where(user_id: keeper)
    beers = beers.where(brew_id: brew) if brew
    beers
  end
end

The Beer.from_cellar could change to

@brew = Brew.find(params[:id])
@beers = @some_collector.beers.stocked.by_brew(@brew) # <- THIS, I hate.

# It can be wrapped into something like
module FetchBeers
  def stocked_beers(brew)
    beers.stocked.by_brew(brew)
  end
end

@some_collector.extend FetchBeers
@beers = @some_collector.stocked_beers(@brew)

Or something, I suppose. Maybe I need more ☕ or 🍺 before free-wheeling something like this. At any rate, we'll get into it more in a couple of weeks at Codemash! Thanks again, buddy!

Yeah, the real problem is that AR is kinda broken by design. Yet so many things are written to work with it, going with a lesser-used ORM can be rough.

@steveklabnik,
Agreed. For this particular app, I really considered going with a Data Mapper approach, but in the end the thought of all of the friction scared me into sticking with AR. Perhaps, in hindsight, the friction I'm feeling in my design w/AR is MORE than would have been felt by going with a different ORM.

Fucking software, it's hard, man.

Fucking software, it's hard, man.

Word. Datamapper is okay, but the Ruby implementation tries to pretend it's AR too much, it has the same problems.

I don't have the time and energy to write a good ORM...

I feel like the DCI pattern could help you here... similar to your last code posting. (As we just discussed offline, but for posterity.)

Let data objects be primarily data objects. AR does an OK job with that anyway. That's the D.

The C would be your use case (I like the term activity better, but anyway). C is for context.

I are your interactions that you extend onto AR's domain/data objects at runtime inside your context.

The DCI pattern puts contextual analysis over the domain modelling desire "single source of truth," so I understand it's a different motivation but there might be more value in that POV?

There's some great discussion here, wish I could make it to CodeMash this year to partake in the booze filled continuation.

I agree AR definitely leaves a lot to be desired when it comes to separation of concerns. It's true that if you follow the idiomatic rules, things can remain quite testable purely because of the powerful stubbing capabilities in Ruby and RSpec. You don't have to wrap everything in a testable abstraction like we were all used to doing with C#.

But there are definitely things that still feel weird with AR and don't work quite as well if you try to build any abstraction layers around it. I'd consider Sequel as a possible alternative to using an ORM, although its really raw by comparison.

Or just try using MongoDB and get out of relational mapping all together. For side projects, I think its probably the best way to go.