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

Shorter, better way to write a "query" involving virtual attributes

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

Problem

I have this code in a model for an invoicing app. This virtual attribute 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
  end


You 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:

-
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
end

Code 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
end

Context

StackExchange Code Review Q#41374, answer score: 4

Revisions (0)

No revisions yet.