2
\$\begingroup\$
params[:controller_id].each do |cid|
  params[:zone_number].each do |pzn|
    zone_ids << Zone.find_by_controller_id_and_number(cid,pzn).id
  end
end

I have multiple controllers and multiples zone_numbers, and I would like to build a single array of zone_ids by iterating through controller and then zone per controller.

The code I wrote works - but there has to be a more eloquent way!

\$\endgroup\$
3
\$\begingroup\$

If you need to do the combination in Ruby (and I don't think you do; read on), you could do something like this, using Array#product

# just extracting these 2 params for clarity
controller_ids = params[:controller_id]
zone_numbers = params[:zone_number]

# product will give you every id/number combo,
# which is then mapped to a Zone record's ID
zone_ids = controller_ids.product(zone_numbers).map do |controller_id, number|
  Zone.find_by_controller_id_and_number(controller_id, number).id
end

However! I'd suggest less Ruby and more SQL for this.

# line-breaks are for clarity
zone_ids = Zone.where(
  'controller_id IN (?) AND number IN (?)', controller_ids, zone_numbers
).pluck(:id)

That should pluck just the id values from the Zones table without loading entire records.

While you can generate an array that contains every combination of controller ID and zone number, you'd still (with your current code) do separate DB queries for each of those combinations. And doing SQL queries in a loop is a sure sign you should look for another approach altogether (namely doing more in the DB layer, if possible).

\$\endgroup\$
  • \$\begingroup\$ The relationships they set up before I arrived don't lend themselves to an eager-loading type query. Fixing the relationships is something we have planned for a later stage with a greater over-haul. The scope of this issue means biting the short-term bullet. Lectures about technical debt are cast in iron in my heart, but it doesn't matter in this case. Must. ship. code. \$\endgroup\$ – notaceo Aug 6 '14 at 21:16
  • 1
    \$\begingroup\$ @notaceo Sure, my point wasn't to throw everything away or anything like that. My "another approach altogether" comment referred only to using SQL instead of more Ruby, as the code in my answer does. I'm unsure though why the above wouldn't work. If you scroll to the end, you'll see pluck call, so it should load only the IDs to an array. No "eager loading" as such, just a bunch of queries condensed into 1. \$\endgroup\$ – Flambino Aug 6 '14 at 22:14
  • \$\begingroup\$ I really like your approach, and I would use it, but I realized after I made the post that I run this method to check if the zone passed exists for the controller - meaning I have to check them all individually per individual controller, which means I do that while I'm building my expensive array... I'm sure there is some good refactoring in there, I just don't have the time to focus on it right now :( \$\endgroup\$ – notaceo Aug 7 '14 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.