1
\$\begingroup\$

I've built-out a mechanism to go and find an auto-generated invoice and update it with new information when a plan is changed or if users are added to the plan for the current billing period. It will make it a lot easier than having to remember to change it a month, quarter, 6 months, or even a year from now. And this also reduces the burden on the finance department from having to go find the future invoice and manually updating it. I hope my comments in the code are clear.

I'm mainly looking for feedback on my code. It does work. I will need to write tests for this though.

Model

def new_amount(max_users, item)
  amount = ((max_users.to_i * item.amount) / item.quantity)
  # Returns the new adjusted amount
  case term_type
  when 'month'      then amount / 12
  when 'quarter'    then amount / 4
  when 'semiannual' then amount / 2
  when 'annual'     then amount / 1
  else                   amount
  end
end

Controller

  # Finds the upcoming / unpaid system invoice.
  manual_invoice = @subscription.manual_invoices.system.not_paid.latest.first

  # TODO: This is very hacky. If I use dup or clone on manual_invoice, and
  #   when change_plan is executed, the calculation runs, regardless if
  #   there were any updates. Also, chaining these scopes brings back all
  #   of the records when using .latest... so, I end up with an invoice
  #   where paid_at is nil.
  first_manual_invoice = @subscription.manual_invoices.system.paid.first

  # Guards changing of plan from blowing up if manual_invoice.nil?
  if manual_invoice
    # Iterate through the invoice's items.
    first_manual_invoice.items.each do |item|
      # Set new_amount by taking the new max_users quantity and multiply it
      #   by the previous amount. Then divide it by the previous max_users
      #   quantity. Finally, divide by payment frequency per term.
      new_amount = @subscription.new_amount(max_users, item)
      # Update the manual_invoice items with the calcuated new_amount and
      #   user quantity.
      manual_invoice.items.first.update_attributes(quantity: max_users,
                                                   amount: new_amount)
      # Update the manual_invoice.
      manual_invoice.update_attributes(amount_in_cents: new_amount,
                                       term_type: term_type,
                                       term_start_date: term_start_date,
                                       term_end_date: term_end_date,
                                       due_on: term_end_date)
    end
  end
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Your code is very readable. Given the provided snippets of code, I won't focus this review on overall architecture (as I'm not sure what the entire system does). I'd like focus on two areas of this code: comments and method names.

When reading your comments I asked my self the following question (taken directly from Code Complete 2).

Do comments explain the code's intent or summarize what the code does, rather than just repeating the code?

# Returns the new adjusted amount
# Iterate through the invoice's items.
# Update the manual_invoice.

These comments repeat what your code does. Removing them would improve the readability and reduce the length of your code.

Next, the following comment and method invocation:

# Set new_amount by taking the new max_users quantity and multiply it
#   by the previous amount. Then divide it by the previous max_users
#   quantity. Finally, divide by payment frequency per term.
new_amount = @subscription.new_amount(max_users, item)

A 3 line comment before a method call such as this is a great opportunity to rename your method name to be more descriptive and possibly avoid a comment altogether. Here is an idea of a possible name:

adjusted_amount = @subscription.adjust_amount(max_users, item)

What other method names would better describe what this method does? Could you move this method to another Model?

Don't be afraid to use 3 or more words to describe a method if that's what it takes. Better to have a long method name instead of a 3-line comment.

I believe you could improve these two lines:

# Guards changing of plan from blowing up if manual_invoice.nil?
if manual_invoice

By replacing them with one line:

unless manual_invoice.nil?

Finally, inside the method new_amount I believe referencing model attributes with self would the improve the readability of your code by differentiating parameters from model attributes.

case self.term_type
# ...
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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