7
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ 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? \$\endgroup\$ – andHapp Feb 11 '14 at 11:45
  • \$\begingroup\$ 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. \$\endgroup\$ – Petr Bela Feb 17 '17 at 12:38
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – niftygrifty Feb 21 '14 at 2:08
  • 1
    \$\begingroup\$ yep.. without your exepction it could just be invoices.detect(&:not_rescinded?) \$\endgroup\$ – natedavisolds Feb 21 '14 at 21:09
4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – niftygrifty Feb 12 '14 at 2:43
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – tokland Feb 12 '14 at 9:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.