# Show all records when current user is admin

I have the following. How best to refactor it:

  def self.search(search, organization_id, is_admin)
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.

• 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}%"))
with_record_no
else
with_record_no.where(organization_id: organization_id)
end
end

• Is sending id instead of object, a bad practice? What if the object is huge? – Victor Feb 1 '16 at 11:02
• "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. – tokland Feb 1 '16 at 11:35

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