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
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
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.
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
So we have at least 5 models involved (
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?