patternrubyrailsMinor
Shorter, better way to write a "query" involving virtual attributes
Viewed 0 times
involvingquerywaywritebetterattributesshortervirtual
Problem
I have this code in a model for an invoicing app. This virtual attribute
You might be wondering why not just do this...
...but current_status is a virtual attribute on invoice.
current_invoice gets the only non-rescinded invoice associated with a contract. The code works but it seems really verbose. Can this be done in a prettier way?def current_invoice
result = []
invoices.each do |i|
if i.current_status != "rescinded"
result.push(i)
else
false
end
end
if result.length > 1
raise "Only one non-rescinded invoice association per contract"
else
result[0]
end
endYou might be wondering why not just do this...
invoices.where.not(current_status: "rescinded").first...but current_status is a virtual attribute on invoice.
Solution
Some notes:
-
-
init empty array + each + conditional push = select/reject. More on functional programming.
-
-
-
I'd write:
-
result = []: Try not to use generic names like result. Also, use plural names for collections.-
init empty array + each + conditional push = select/reject. More on functional programming.
-
else false. In an each block the value is ignored, so this does nothing.-
result[0]. People usually write result.first, but [0] is also ok.-
if result.length > 1. There is a serious conceptual problem here, validations should be enforced somewhere else (this is Rails, so using a validation callback). I'd write:
def current_invoice
non_rescinded_invoices = invoices.select { |i| i.current_status != "rescinded" }
if non_rescinded_invoices.size > 1 # but this should be probably an AR validation
raise("Only one non-rescinded invoice association per contract")
else
non_rescinded_invoices.first
end
endCode Snippets
def current_invoice
non_rescinded_invoices = invoices.select { |i| i.current_status != "rescinded" }
if non_rescinded_invoices.size > 1 # but this should be probably an AR validation
raise("Only one non-rescinded invoice association per contract")
else
non_rescinded_invoices.first
end
endContext
StackExchange Code Review Q#41374, answer score: 4
Revisions (0)
No revisions yet.