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

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.

• I know you said current_status is virtual, but is it initialised based on a value in the database, perhaps in another model and may be you could use that to create a query? – andHapp Feb 11 '14 at 11:45
• Related to the previous comment, if you could filter on the database level, it would be much faster than fetching all invoices and then filtering them in Ruby. – Petr Bela Feb 17 '17 at 12:38

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


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

• Plus one for the helpful alternative! But I'm trying to get invoices that were not rescinded so probably best to change the variable name. Also, would this be alright in the current_invoice method: invoices.reject(&:rescinded?).first? Without using a variable name at all if I were to ditch the exception. – niftygrifty Feb 21 '14 at 2:08
• yep.. without your exepction it could just be invoices.detect(&:not_rescinded?) – natedavisolds Feb 21 '14 at 21:09

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

• Thanks for this great answer. I actually am checking that no invoices be created if one non-rescinded invoice exists as an ActiveRecord validation, but I thought it might be a good idea to double check it here and raise an exception. What do you think about that? Is it unnecessary duplication? – niftygrifty Feb 12 '14 at 2:43
• Yes, I think it's unnecessary, you need to trust your own code :) Personally I don't want a "read-only" method like this to raise an error regarding validations. Another matter would be if you wanted to do some asserts on the arguments of a method, some sanity checks here and there are ok. – tokland Feb 12 '14 at 9:31