8

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?

closed as too broad by Scant Roger, Michael Kohne, Ixrec Dec 4 '15 at 8:06

Please edit the question to limit it to a specific problem with enough detail to identify an adequate answer. Avoid asking multiple distinct questions at once. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 6
    You have mentioned only one aspect of this code that is broken: it gives a 500 error when payment fails. This should be fairly easy to fix without redesigning the whole thing, though. Are there other concrete reasons this code needs to be rewritten? Ugly code that works should usually be left at it is, unless there is a strong rationale for changing it. As you rightly worry, redesign takes a lot of effort and may introduce new problems. – user82096 Nov 30 '15 at 6:32
  • It would seem easier to fix the 500 page but the difficulty stems from the poor design. The 500 page is due to an AASM::InvalidTransition: Event 'process' cannot transition from 'failed' exception which masks the real error which is an unsuccessful transaction. There is so much indirection it's hard to get the response back to the user and allow a resubmission. I'm sure it's possible but it seems almost as difficult as refactoring. – Reed G. Law Nov 30 '15 at 6:58
  • 2
    "and greped every method to find out where they're called there's a high probability I will miss something" - sounds like part of the problem is the fact you are using a language where no compiler can tell you exactly where a method is called. In such a situation, you probably have to resist the wish to refactor, how much sense it will ever make. – Doc Brown Nov 30 '15 at 21:16
20

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.

  • 2
    The quote from Joel's article doesn't apply in this case because I know the current use cases and that a lot of the code is indeed unnecessary. As far as increasing test coverage to 100%, I spent many months getting coverage up to about 80%. The remaining uncovered code is either less critical, harder to test, or both. Even with reported 100% test coverage I wouldn't trust automated tests to catch every possible edge case unless I spent an order of magnitude more time writing tests. Another rational for refactoring is that every time this section is visited it takes a long time to understand. – Reed G. Law Nov 30 '15 at 7:09
  • 3
    @ReedG.Law, one further possibility is to simplify the logic internally, but not change the interface that is exposed to the rest of the code--sort of a halfway redesign that is lower risk. This would still carry some risk of introducing new problems though. – user82096 Nov 30 '15 at 7:16
  • that's a possibility, but there is a large surface area to the interface because of tightly coupled code. I might be able to get rid of the state machine in Charge at least. – Reed G. Law Nov 30 '15 at 7:31
  • 2
    @ReedG.Law as an aside, I am surprised that this code would be embedded lots of different places in the site. Most ecommerce sites have a single defined "checkout" path, and I would only expect payment code to be called in that one location. If that isn't the case, maybe the calling code is really what needs attention, before this code? If you really do need to redesign, maybe the process is: 1) make sure the whole site directs to a single path for checkout. 2) after you have done that, you can safely redesign the payment code. – user82096 Nov 30 '15 at 7:46
  • 3
    @ReedG.Law - you say because I know the current use cases, yet your original post states Each of the redundant classes has many methods that could be called from anywhere in the code base . Those 2 quotes appear to be mutually exclusive. – Kickstart Nov 30 '15 at 12:43

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