6

I have many multi-line ActiveRelation query methods in our app, and I'm unsure about the most idiomatic way to write these methods. Take a look at this example:

def postal_code_ids_within(miles)
  nearby_postal_codes = PostalCode.where("latitude > :min_lat and latitude < :max_lat",
    min_lat: (latitude - (miles.to_f / MILES_PER_DEGREE_LATITUDE.to_f / 2.to_f)),
    max_lat: (latitude + (miles.to_f / MILES_PER_DEGREE_LATITUDE.to_f / 2.to_f)))
  nearby_postal_codes = nearby_postal_codes.where("longitude > :min_lon and longitude < :max_lon",
    min_lon: (longitude - (miles.to_f / MILES_PER_DEGREE_LONGITUDE.to_f / 2.to_f)),
    max_lon: (longitude + (miles.to_f / MILES_PER_DEGREE_LONGITUDE.to_f / 2.to_f)))
  nearby_postal_codes.pluck(:id)
end

It feels a bit off to me. A block from which an ActiveRelation object is returned seems idiomatic, but I haven't seen that approach around.

What is standard?

barelyknown
  • 5,510
  • 3
  • 34
  • 46
  • 1
    If you used local variables to store lat and long values, it would probably be a bit more legible. – Robin Aug 20 '12 at 15:30
  • 1
    One approach you may consider is breaking this up into scopes: http://guides.rubyonrails.org/active_record_querying.html#scopes – Brian Aug 20 '12 at 15:35
  • Any idea why the guide recommends class methods instead of scopes with arguments? Scopes with arguments are nice because it's clear that they return ActiveRelation objects, and I'm not sure what the downside is. – barelyknown Aug 20 '12 at 16:02

2 Answers2

11

Building on Brian's suggestion, this is much more legible and works well.

scope :near, lambda { |postal_code, miles|
  degree_offset = miles / MILES_PER_DEGREE / 2
  where("latitude > :min_lat and latitude < :max_lat and longitude > :min_lon and longitude < :max_lon",
    min_lat: postal_code.latitude - degree_offset,
    max_lat: postal_code.latitude + degree_offset,
    min_lon: postal_code.longitude - degree_offset,
    max_lon: postal_code.longitude + degree_offset)
}

def postal_code_ids_within(miles)
  self.class.near(self, miles).pluck(:id)
end
barelyknown
  • 5,510
  • 3
  • 34
  • 46
  • 1
    It's a personal preference, but I think using `do ... end` with that lambda block could be considered more appropriate for multi-line blocks. – Brian Aug 20 '12 at 16:09
  • That's what I actually had, but that syntax through an error (not sure why). I suppose that I might have had a typo, but I don't think that I did. Could that make sense? – barelyknown Aug 20 '12 at 19:47
  • 2
    Bad advice on my part (at least in this case). See this thread - it seems the syntax error is likely due to precedence issues: http://stackoverflow.com/questions/1476678/rails-named-scope-lambda-and-blocks – Brian Aug 20 '12 at 20:12
0

Breaking up into scopes is a good suggestion. In contrast to another answer I prefer defining complicated scopes as a function instead of messing with lambdas, blocks and precedence rules:

def self.near(postal_code, miles)
  degree_offset = miles / MILES_PER_DEGREE / 2
  where("latitude > :min_lat and latitude < :max_lat and longitude > :min_lon and longitude < :max_lon",
    min_lat: postal_code.latitude - degree_offset,
    max_lat: postal_code.latitude + degree_offset,
    min_lon: postal_code.longitude - degree_offset,
    max_lon: postal_code.longitude + degree_offset)
end

def postal_code_ids_within(miles)
  self.class.near(self, miles).pluck(:id)
end
Community
  • 1
  • 1
geekQ
  • 29,027
  • 11
  • 62
  • 58