I can see three problems with this example.
1. You are doing DB queries in the View template
A simple rule of thumb with Rails views: avoid triggering DB queries in your views at all costs.
The Rails convention is to do all your data preparation in the Controller, and your Views should only use what has already been gathered.
An exception to this rule is lazy-loaded data, but that would still be initialized somewhere else.
2. You are performing queries in a loop
By performing a DB query inside a loop, you are generating far more requests than is likely necessary.
3. You are (it appears) relying on a uniqueness constraint in the model
Never rely on validation alone to prevent duplicate records in your database. It is possible that you've already accounted for this and simply omitted the relevant code, but I want to make that point in case you haven't because it is important.
Honestly it seems like you should find a way to make
abbr an actual foreign key so that you can rely on the framework to do lookups for you. Non-numeric keys are a thing. This looks like a standard many-many relationship, why are you reinventing the wheel here?
If that is not possible, here's how I would approach this:
@device_vendors = DeviceVendor.order(:device, :vendor)
@devices = Device.where(abbr: @device_vendors.map(&:device).uniq).index_by(&:abbr)
@vendors = Vendor.where(abbr: @device_vendors.map(&:vendor).uniq).index_by(&:abbr)
index_by in this way creates a Hash keyed to the
abbr value. If
abbr is non-unique and there are multiple records with the same value, this gets slightly more complicated but the principle is the same: pull in every record you need up front with as few queries as feasible.
<% @device_vendors.each do |device_vendor| %>
<td><%= @devices[device_vendor.device].try(:name) %></td>
<td><%= @vendors[device_vendor.vendor].try(:name) %></td>
<td><%= link_to 'Show', device_vendor %></td>
<td><%= link_to 'Edit', edit_device_vendor_path(device_vendor) %></td>
<% end %>
try covers the case where the
abbr key wasn't found.