HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Show all records when current user is admin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
showadminalluserrecordscurrentwhen

Problem

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.

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#118480, answer score: 4

Revisions (0)

No revisions yet.