Use an existing invoice if one exists, else create one

Question

Here are the requirements:

• If initial_invoice? is false, existing_invoice always exists.
• If initial_invoice? is true and existing_invoice exists, I'd love to use the existing_invoice.

I'd love to refactor this code.

 existing_invoice = find_existing_invoice(invoice_obj[:id])
   invoice = if !initial_invoice?(invoice_obj) || existing_invoice.present?
                existing_invoice
              else
                ac = Account.find_by(stripe_customer_id: invoice_obj[:customer])
                create_initial_invoice(invoice_obj, ac)
              end

  private

  def find_existing_invoice(id)
    Invoice.find_by(stripe_invoice_id: id)
  end

  def initial_invoice?(invoice_obj)
    sub_id = invoice_obj[:subscription]
    return false if sub_id.nil?

    sub = Stripe::Subscription.retrieve(sub_id)
    return false unless invoice_obj[:period_start] == sub[:created]
    true
  end

update

invoice = if initial_invoice?(invoice_obj)
            return existing_invoice if existing_invoice.present?

            ac = Account.find_by(stripe_customer_id: invoice_obj[:customer])
            create_initial_invoice(invoice_obj, ac)
          else
            existing_invoice
          end

Answer 1

The refactor goal was to make clear the logic for getting an invoice.

• THIS IS THE MOST IMPORTANT POINT: Original code tells me there are 2 kinds of invoices so I knew I wanted a "if" that stated very simply and high level the logic for deciding which. So I wrote that and worked all the rest of the code around it.
• Moving code into the get_xxx methods I saw that fetching the existing invoice was in both methods.
• The above had me realize that no matter what, we use an pre-existing invoice if it's there. So I pulled that line out and put it at the top. Now it is obvious what's really going on.
• I renamed invoice_obj because that sounds like it's an invoice too. But it's not. Its properties are used for fetching an actual invoice. So I thought of invoice_obj as metadata of a real invoice.
• Make both "if" branches the same "level of abstraction". I'm trying to express that there are 2 kinds of invoices. I'm hiding the detail code that actually does it - that's a different level of detail.

I want it to say this:

if ...
  get_initial_invoice(metadata)
else
  get_invoice(metadata)

and not this:

if ...
  get_initial_invoice(metadata)
else
  Invoice.find_by(metadata[:id])

---

Refactored Code

 return existing_invoice if existing_invoice.present?

 invoice = nil

 if initial_invoice?(invoice_metadata)
    invoice = get_initial_invoice(invoice_metadata)
 else
    invoice = get_invoice(invoice_metadata)
 end

 return invoice


 private

 def get_initial_invoice(invoice_metadata)   
    ac = Account.find_by(stripe_customer_id: invoice_metadata[:customer])
    create_initial_invoice(invoice_metadata, ac)
 end

 def get_invoice(invoice_metadata)
    return Invoice.find_by(invoice_metadata[:id])
 end

 def initial_invoice?(invoice_metadata)
   return false if invoice_metadata[:subscription].nil?

   sub = Stripe::Subscription.retrieve(invoice_metadata[:subscription])
   invoice_metadata[:period_start] == sub[:created] ? true : false
 end