4
\$\begingroup\$

Is this much logic in view is justified to check for the presence of record?

# device model
class Device < ApplicationRecord
  validates :abbr, uniqueness: { case_sensitive: false }, presence: true
  validates :name, presence: true

  def abbr=(value)
    self[:abbr] = value.to_s.strip
  end
end

Here is the code for DeviceVendor that also saves abbr column of Device to uniquely identify the device:

# model code 
class DeviceVendor < ApplicationRecord
  validates :vendor, uniqueness: {scope: :device}
end

# controller code
def index
  @device_vendors = DeviceVendor.order(:device, :vendor)
end

# view code
<% @device_vendors.each do |device_vendor| %>
  <% device = Device.where(abbr: device_vendor.device).first %>
  <% vendor = Vendor.where(abbr: device_vendor.vendor).first %>
  <tr>
    <td><%= device.name if device %></td>
    <td><%= vendor.name if vendor %></td>
    <td><%= link_to 'Show', device_vendor %></td>
    <td><%= link_to 'Edit', edit_device_vendor_path(device_vendor) %></td>
  </tr>
<% end %>

What is the rails way to do this?

\$\endgroup\$
  • \$\begingroup\$ What are the associations between Device and DeviceVendor? \$\endgroup\$ – Pavan Jun 13 '17 at 11:06
  • \$\begingroup\$ Yes, it is too much logic in views. \$\endgroup\$ – Sergio Tulentsev Jun 13 '17 at 11:17
  • \$\begingroup\$ @Pavan Currently Device has abbr column which will be unique and this will be saved in DeviceVendor not device_id \$\endgroup\$ – Rajkaran Mishra Jun 13 '17 at 11:25
  • \$\begingroup\$ Just update the associations please \$\endgroup\$ – Pavan Jun 13 '17 at 11:27
  • \$\begingroup\$ @devel check my answer, it will work. \$\endgroup\$ – puneet18 Jun 13 '17 at 12:59
2
\$\begingroup\$

There's a lot of ways to check the presence of an object, i prefer to use try :

device.try(&:name)
\$\endgroup\$
  • 2
    \$\begingroup\$ try doesn't accept a block, though. \$\endgroup\$ – Sergio Tulentsev Jun 13 '17 at 13:09
2
\$\begingroup\$

You could also use first_or_initialize. It tries to find the record and, if not found, creates a new unsaved object (so that you always have an object and don't have to branch. This is called "confident code").

<% device = Device.where(abbr: device_vendor.device).first_or_initialize %>
<% vendor = Vendor.where(abbr: device_vendor.vendor).first_or_initialize %>
<tr>
  <td><%= device.name %></td>
  <td><%= vendor.name %></td>

Although I'm not a fan of queries in views like that. You should make proper activerecord relations there. Or, at least, move the queries to DeviceVendor's methods.

<td><%= device_vendor.device_or_default.name %></td>
<td><%= device_vendor.vendor_or_default.name %></td>
\$\endgroup\$
2
\$\begingroup\$

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:

Controller

def index
  @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)
end

Using 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.

View

<% @device_vendors.each do |device_vendor| %>
  <tr>
    <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>
  </tr>
<% end %>

Using try covers the case where the abbr key wasn't found.

\$\endgroup\$
  • \$\begingroup\$ "You are creating an n+2 query problem" - it's 2N + 1, actually :) \$\endgroup\$ – Sergio Tulentsev Jun 15 '17 at 18:47
  • \$\begingroup\$ @SergioTulentsev I should have known invoking O-notation would bite me. \$\endgroup\$ – Adam Lassek Jun 16 '17 at 23:36
1
\$\begingroup\$

Is this much logic in view is justified to check for the presence of record?

It is justified to check for a presence of a record in the view but you are writing AR queries in the view which will trigger DB calls and its not the Rails way to do it! You should instead move those to corresponding view helper or define it as model methods. I would do

#view_helper
def get_device_name_and_vendor_name(dv)
  device = Device.where(abbr: dv.device).first
  device_name = device.blank? ? nil : device.name
  vendor = Vendor.where(abbr: dv.vendor).first
  vendor_name = vendor.blank? ? nil : vendor.name
  return device_name,vendor_name
end

And in the view, call that method to get device name and vendor name

<td><%= get_device_name_and_vendor_name(device_vendor)[0] %></td>
<td><%= get_device_name_and_vendor_name(device_vendor)[1] %></td>

Note: I would also instead apply associations to the models to cleanup those queries.

\$\endgroup\$
  • \$\begingroup\$ Can you also explain how association can be applied in this scenario as device_id is not getting used so that I can seed all the data to avoid association \$\endgroup\$ – Rajkaran Mishra Jun 13 '17 at 12:49
  • \$\begingroup\$ @devel Associations in your case is a long shot,since you need to add corresponding Foreign Key in the tables and the data in the db need to be altered too to map the corresponding records. But they provide more flexibility. My suggestion, take it or leave it :) \$\endgroup\$ – Pavan Jun 13 '17 at 12:58
  • \$\begingroup\$ @devel: foreign key column does not have to be device_id. You can customize it. Something along the lines of belongs_to :device, foreign_key: :abbr. Consult docs for the exact syntax. \$\endgroup\$ – Sergio Tulentsev Jun 13 '17 at 13:04
  • \$\begingroup\$ @Pavan: you realize your proposed code makes 4 queries? \$\endgroup\$ – Sergio Tulentsev Jun 13 '17 at 13:11
  • 1
    \$\begingroup\$ @SergioTulentsev Yeah! Since you already pointed out in your answer, I have to think of another way and the final result is this answer :) \$\endgroup\$ – Pavan Jun 13 '17 at 13:30
1
\$\begingroup\$

Using try method of ruby, exception will not be raised and nil will be returned instead, if the receiving object is a nil object.

Try below code in view:

# view code
<% @device_vendors.each do |device_vendor| %>
  <% device = Device.where(abbr: device_vendor.device).first %>
  <% vendor = Vendor.where(abbr: device_vendor.vendor).first %>
  <tr>
    <td><%= device.try(:name) %></td>
    <td><%= vendor.try(:name) %></td>
    <td><%= link_to 'Show', device_vendor %></td>
    <td><%= link_to 'Edit', edit_device_vendor_path(device_vendor) %></td>
  </tr>
<% end %>

OR correct way is to add association

Device has_many_and_belongs_to Vendor
Vendor has_many_and_belongs_to Device

DeviceVendor is Join table of Device and Vendor

\$\endgroup\$
  • \$\begingroup\$ So whats the difference between your answer and Othmane El kesri's answer? \$\endgroup\$ – Pavan Jun 13 '17 at 12:57
  • \$\begingroup\$ @Pavan I am explaining whats the try method of ruby and how to use it. \$\endgroup\$ – puneet18 Jun 13 '17 at 12:59
  • 1
    \$\begingroup\$ @SergioTulentsev Thanks for your suggestion. I updated my answer \$\endgroup\$ – puneet18 Jun 13 '17 at 13:12
0
\$\begingroup\$

You can still create relationships on keys that are not called id (although I wouldn't recommend it. I would recommend using id, at some point someone will want to change an abbreviation and it will not be easy. Joins on integers are also faster than joins on strings). I also suggest you name your join fields on DeviceVendor as device_abbr and vendor_abbr so that you can add logical relationships called device and vendor without causing a conflict.

If you want to join on abbreviation you can do something like:

class Device < ApplicationRecord
  validates :abbr, uniqueness: { case_sensitive: false }, presence: true
  validates :name, presence: true

  has_many :device_vendors, foreign_key: :vendor_abbr, primary_key: :abbr
  # You can do something similar with has_and_belongs_to_many

  def abbr=(value)
    self[:abbr] = value.to_s.strip
  end
end

class DeviceVendor < ApplicationRecord
  validates :device_abbr, presence:true
  validates :vendor_abbr, presence:true, uniqueness: {scope: :device_abbr}
  belongs_to :vendor, foreign_key: :vendor_abbr, primary_key: :abbr
  belongs_to :device, foreign_key: :device_abbr, primary_key: :abbr
end

# controller code
def index
  @device_vendors = DeviceVendor.order(:device, :vendor).include(:device, :vendor)
  # You could also simplify the above with a `scope` on DeviceVendor
end

# view code
<% @device_vendors.each do |device_vendor| %>
  <tr>
    <td><%= device_vendor.device.name if device_vendor.device %></td>
    <td><%= device_vendor.vendor.name if device_vendor.vendor %></td>
    <td><%= link_to 'Show', device_vendor %></td>
    <td><%= link_to 'Edit', edit_device_vendor_path(device_vendor) %></td>
  </tr>
<% end %>
\$\endgroup\$
0
\$\begingroup\$

Another solution would be to add device_name and vendor_name methods to the DeviceModel and encapsulate that logic there.

\$\endgroup\$

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.