patternrubyrailsMinor
Show all records when current user is admin
Viewed 0 times
showadminalluserrecordscurrentwhen
Problem
I have the following. How best to refactor it:
Basically this means, I want it to show all records without the
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
endBasically 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
-
-
-
DRY your code by reusing the repeated scope.
-
-
I'd write:
-
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
endCode 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
endContext
StackExchange Code Review Q#118480, answer score: 4
Revisions (0)
No revisions yet.