Refactoring byzantine payment processing code on a limited budget

Question

I've been working on a large Ruby on Rails application for several years. It was inherited in a poor state but most of the production bugs have been ironed out with time. There are some sections that have not been touched such as the payment processing code. The code works for the most part, except that whenever a charge is denied by the payment processor, the user gets a 500 error instead of a helpful message. I'd like to refactor the code to make it easier to maintain. I'll provide a brief run-down of how it works.

I've removed all the error-handling code from the following snippets.

The maze begins in a controller:

  def submit_credit_card
    ...
    @credit_card = CreditCard.new(params[:credit_card].merge(:user => @user))
    @credit_card.save
    ...
    @submission.do_initial_charge(@user)
    ...
  end

Then in the Submission model:

  def do_initial_charge(user)
    ...
    self.initial_charge = self.charges.create(:charge_type => ChargeType.find(1), :user => user)
    self.initial_charge.process!
    self.initial_charge.settled?
  end

In the Charge model:

  aasm column: 'state' do
    ...
    event :process do
      transitions :from => [:created, :failed], :to => :settled, :guard => :transaction_successful?
    end
    ...
  end

  def initialize(*params)
    super(*params)
    ...
    self.amount = self.charge_type.amount
  end

  def transaction_successful?
    user.reload
    credit_card = CreditCard.where(user_id: user_id).last
    cct = self.cc_transactions.build(:user => user, :credit_card => credit_card, :cc_last_four => credit_card.num_last_four, :amount => amount, :charge_id => id)
    cct.process!
    if self.last_cc_transaction.success
      self.update_attribute(:processed, Time.now)
      return true
    else
      self.fail!
      return false
    end
  end

There are a lot of questionable bits above such as reloading the user and finding the last CreditCard rather than passing in the one just saved. Also this code depends on a ChargeType loaded from the database with a hard-coded ID.

In CcTransaction we continue down the trail:

  def do_process
    response = credit_card.process_transaction(self)
    self.authorization = response.authorization
    self.avs_result    = response.avs_result[:message]
    self.cvv_result    = response.cvv_result[:message]
    self.message       = response.message
    self.params        = response.params.inspect
    self.fraud_review  = response.fraud_review?
    self.success       = response.success?
    self.test          = response.test
    self.response      = response.inspect
    self.save!
    self.success
  end

All this appears to do is save a record in the cc_transactions database table. The actual payment processing is performed in the CreditCard model. I won't bore you with the details of that class. The actual work is performed by ActiveMerchant::Billing::AuthorizeNetCimGateway.

So we have at least 5 models involved (Submission, Charge, ChargeType, CcTransaction, and CreditCard). If I were to do this from scratch, I would only use a single Payment model. There are only 2 charge types, so I would hard code those values as class variables. We don't store credit card details, so that model is unnecessary. Transaction info can be stored in the payments table. Failed payments do not need to be saved.

I could go in and do this refactoring fairly easily except for the requirement that nothing should ever go wrong on the production server. Each of the redundant classes has many methods that could be called from anywhere in the code base. There is a suite of integration tests but the coverage is not 100%.

How should I go about refactoring this while ensuring nothing breaks? If I went through the 5 payment classes and greped every method to find out where they're called there's a high probability I will miss something. The client is already used to how the current code runs and introducing any new bugs is unacceptable. Apart from increasing test coverage to 100%, is there any way to refactor this with certainty that nothing will break?

Answer 1

Consider whether this code really needs refactoring. The code may be ugly and aesthetically displeasing to you as a software developer, but if it works, you probably shouldn't redesign it. And based on your question, it sounds like the code largely works.

This classic article from Joel on Software highlights all of the risks of unnecessarily rewriting code. It is a costly, but very tempting, mistake. The whole thing is worth reading before, but this passage about seemingly unnecessary complexity seems particularly appropriate:

Back to that two page function. Yes, I know, it's just a simple function to display a window, but it has grown little hairs and stuff on it and nobody knows why. Well, I'll tell you why: those are bug fixes. One of them fixes that bug that Nancy had when she tried to install the thing on a computer that didn't have Internet Explorer. Another one fixes that bug that occurs in low memory conditions. Another one fixes that bug that occurred when the file is on a floppy disk and the user yanks out the disk in the middle. That LoadLibrary call is ugly but it makes the code work on old versions of Windows 95.

Each of these bugs took weeks of real-world usage before they were found. The programmer might have spent a couple of days reproducing the bug in the lab and fixing it. If it's like a lot of bugs, the fix might be one line of code, or it might even be a couple of characters, but a lot of work and time went into those two characters.

Yes, the code is needlessly complex. But still, some of the steps that seem unnecessary may be there for a reason. And if you go to rewrite it, you will have to learn all of those lessons over again.

Reloading the user, for example, seems pointless: which is exactly why you should worry about changing it. No developer (even a bad one) would have introduced a step like that in their initial design. It was almost certainly put in there to fix some problem. Maybe it is a problem that refactoring to the "correct" design will eliminate...but maybe not.

Also, another small point, I'm not convinced by your contention that it is unnecessary to save failed payments. I can think of two strong reasons for doing so: logging evidence of possible fraud (e.g. someone trying various card numbers), and customer support (a customer claims they keep trying to pay, and it is not working...you would want evidence of any attempted payments to help troubleshoot this). This makes me think that perhaps you haven't fully thought through the requirements of the system, and they aren't as simple as you believe they are.

Fix individual problems without fundamentally changing the design. You mentioned one problem: there is a 500 error when the payment fails. This problem should be easy to fix, probably just by adding one or two lines at the right location. It isn't sufficient rationale for tearing everything apart to make the "correct" design.

There may be other problems, but if the code works 99% of the time right now, it is unlikely that these are going to be fundamental problems requiring a rewrite, either.

If the current design is embedded throughout the code, then even spending a fair amount of effort to fix a problem without changing the design may be warranted.

In some cases it may, indeed, be necessary to do a larger redesign. But this would require a stronger rationale that you have given so far. For example, if you plan to develop the payment model further and introduce significant new features, cleaning up the code would have some benefit. Or perhaps the design contains a fundamental security flaw that needs to be fixed.

There are reasons to refactor a large chunk of code. But if you do that, increase the test coverage to 100%. This is probably something that will save you time overall.