3
\$\begingroup\$

I have the following. How best to refactor it:

  def self.search(search, organization_id, is_admin)
    if is_admin == false
      where("record_no LIKE ?", "%#{search}%").
        where("organization_id = ?", organization_id)
    else
      where("record_no LIKE ?", "%#{search}%")
    end
  end

Basically this means, I want it to show all records without the organization_id when user is an admin.

\$\endgroup\$
4
\$\begingroup\$

About your code:

  • The argument name search is confusing (the same as the method), use instead the value it stands for: record_no.

  • "record_no LIKE ?". This is perfectly fine, but, if you used Arel instead, the query would work for all database.

  • if is_admin == false -> if !is_admin or unless is_admin. Shorter, more declarative, idiomatic.

  • DRY your code by reusing the repeated scope.

  • organization_id: Since you using anORM, you usually send objects as arguments, not their IDs. Does it make sense for you code?

  • where("organization_id = ?", organization_id). Use a hash instead.

I'd write:

def self.search(record_no, organization_id, is_admin)
  with_record_no = where(arel_table[:record_no].matches("%#{record_no}%"))
  if is_admin
    with_record_no
  else
    with_record_no.where(organization_id: organization_id)
  end 
end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is sending id instead of object, a bad practice? What if the object is huge? \$\endgroup\$ – Victor Feb 1 '16 at 11:02
  • \$\begingroup\$ "Sending" an object is not a problem, you are passing a reference. The thing is: if you have the object already instantiated when you call this method or you have just the ID. Then you decide which signature to use. \$\endgroup\$ – tokland Feb 1 '16 at 11:35
1
\$\begingroup\$

You could do this:

def self.search(search, organization_id, is_admin)
  # Avoids repetition of this query. Take note that Rails doesn't hit
  # the database until hits an iterator or other non-AR method
  records = where("record_no LIKE ?", "%#{search}%")

  # if the user is admin we could return the records
  return records if is_admin

  # Adding more conditions to the query when the user is not admin
  records.where(organization_id: organization_id)
end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – SuperBiasedMan Feb 1 '16 at 9:28
  • 1
    \$\begingroup\$ @SuperBiasedMan thank you for pointing out. I've added some comments \$\endgroup\$ – Mario Pérez Feb 1 '16 at 9:48
  • 1
    \$\begingroup\$ Great! Just to mention, it's often more readable to have the notes not just as comments in the code but rather plain text in between the code you're demonstrating. \$\endgroup\$ – SuperBiasedMan Feb 1 '16 at 10:07

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.