Shorter, better way to write a “query” involving virtual attributes

Question

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.

Answer 1

Here's what I would change:

1. The else false isn't being used for anything

2. Move "rescinded" logic to invoice instead of querying invoice and then comparing with a string.

3. use Array#reject to eliminate rescinded invoices

Add method to the Invoice class

class Invoice
  def rescinded?
    current_status == "rescinded"
  end
end

and then in your method

def current_invoice
  rescinded_invoices = invoices.reject &:rescinded?

  raise "Only one non-rescinded invoice association per contract" if rescinded_invoices.length == 1

  rescinded_invoices.first
end

EDIT for clarity:

def current_invoice
  non_rescinded_invoices = invoices.reject &:rescinded?

  raise "Only one non-rescinded invoice association per contract" if non_rescinded_invoices.length > 1

  non_rescinded_invoices.first
end

Answer 2

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