2
\$\begingroup\$

I've been working on a large Ruby on Rails application for several years. We inherited it and refactored and upgraded it, but there are some sections that have not been touched in at least 5 years (parts created by the original developer(s)). One such section is payment processing code. The code works for the most part, so we leave it untouched. The only problem is that whenever a charge is denied by the payment processor, the user gets a 500 error instead of a helpful message.

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?

\$\endgroup\$
  • \$\begingroup\$ Unfortunately I don't have an answer for you, but I'm very interested to see where this goes. However, how can you possibly break it if it's your program...? \$\endgroup\$ – 13aal Nov 30 '15 at 2:04
  • \$\begingroup\$ @LostBam I mean making changes can cause errors on the production server. We can test changes on a staging server but there's only so many ways to test manually. The production server gets thousands of real transactions and we can't just replay them all to see if it's working. The difficulty is compounded by the fact that we don't get real error messages from the payment gateway in a staging environment. The only way to get real responses is to use real credit cards in live mode which means each test costs real money and has to be manually refunded. \$\endgroup\$ – Reed G. Law Nov 30 '15 at 2:41
  • 2
    \$\begingroup\$ The first step in refactoring is writing unit tests. If you are unable to figure out how to write unit tests, then you may need to ask a specific question on Stack Overflow or Software Engineering. \$\endgroup\$ – 200_success Nov 30 '15 at 5:04
  • \$\begingroup\$ @200_success All the code in the question is working. The problem is that it's inflexible due to unnecessary complexity. I mentioned testing in the question. \$\endgroup\$ – Reed G. Law Nov 30 '15 at 5:21
  • 2
    \$\begingroup\$ @200_success I've posted it here: programmers.stackexchange.com/questions/303879/… \$\endgroup\$ – Reed G. Law Nov 30 '15 at 5:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.