# Use an existing invoice if one exists, else create one

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

• existing_invoice.present? - is existing_invoice potentially nil? ... Can initial_invoice? be in the Invoice class so you can do this: existing_invoice.initial? ... Should the requirement "... existing_invoice always exists" be explicitly checked up front; and then handle as an error if not? ... Is create_initial_invoice different, really, from creating any invoice? Why the "special" method? If the invoice class had a initial? property then may not need special methods, only find_invoice and create_invoice. Might DRY up some code and make client (caller) code simpler. – radarbob Jan 5 '17 at 3:59
• @radarbob Thanks for the advice. This is a Webhook handler. So depending on the values, I need to execute differently. (create invoice and create invoice_items) – Peco Jan 5 '17 at 4:10
• Invoice.find_by finds initial invoices, and Account.find_by finds all other invoices? Yes? I'd write the top-level logic around this to make the difference clear, like, "IF initial_invoice then create-or-find initial-invoice ELSE create-or-find not-initial-invoice END." That emphasizes what I think is the real difference. Then checking/validating those requirements, if needed, are in proper context of the particular "create-or-find" method. In any case it really troubles me that I need a second object to tell me if I'm an initial_invoice. I should know that for myself. – radarbob Jan 5 '17 at 4:59
• updated! Is this more obvious? It would be more appreciate if you could post your code. – Peco Jan 5 '17 at 6:25
• I hear ya dude. We come to StackExchange for answers. I was past my beddy-bye time last night and I'm home sick today. You're right that this code needs work; it also needs some thought. I did not want to simply regurgitate a half-ass an answer. – radarbob Jan 5 '17 at 16:44

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 ...
else


and not this:

if ...
else


Refactored Code

 return existing_invoice if existing_invoice.present?

invoice = nil

else
end

return invoice

private