1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – radarbob Jan 5 '17 at 3:59
  • \$\begingroup\$ @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) \$\endgroup\$ – Peco Jan 5 '17 at 4:10
  • \$\begingroup\$ 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. \$\endgroup\$ – radarbob Jan 5 '17 at 4:59
  • \$\begingroup\$ updated! Is this more obvious? It would be more appreciate if you could post your code. \$\endgroup\$ – Peco Jan 5 '17 at 6:25
  • \$\begingroup\$ 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. \$\endgroup\$ – radarbob Jan 5 '17 at 16:44
3
\$\begingroup\$

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
\$\endgroup\$

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.