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  endupdateinvoice = 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
Use an existing invoice if one exists, else create one
ruby;ruby on rails
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