Ho lavorato su una grande applicazione Ruby on Rails per diversi anni. È stato ereditato in cattivo stato, ma la maggior parte degli errori di produzione sono stati risolti con il tempo. Ci sono alcune sezioni che non sono state toccate come il codice di elaborazione dei pagamenti. Il codice funziona per la maggior parte, tranne che ogni volta che un addebito viene negato dal processore di pagamento, l'utente riceve un errore 500 invece di un messaggio utile. Mi piacerebbe refactoring il codice per renderlo più facile da mantenere. Fornirò una breve descrizione di come funziona.
Ho rimosso tutto il codice di gestione degli errori dai seguenti snippet.
Il labirinto inizia in un controller:
def submit_credit_card
...
@credit_card = CreditCard.new(params[:credit_card].merge(:user => @user))
@credit_card.save
...
@submission.do_initial_charge(@user)
...
end
Quindi nel modello Submission :
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
Nel modello Charge :
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
Ci sono un sacco di bit discutibili sopra come ricaricare user e trovare l'ultimo CreditCard piuttosto che passare in quello appena salvato. Anche questo codice dipende da un ChargeType caricato dal database con un ID hard-coded.
In CcTransaction continuiamo lungo il percorso:
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
Tutto ciò sembra fare è salvare un record nella tabella di database cc_transactions . L'elaborazione del pagamento effettiva viene eseguita nel modello CreditCard . Non ti annoierò con i dettagli di quella classe. Il lavoro effettivo viene eseguito da ActiveMerchant::Billing::AuthorizeNetCimGateway .
Quindi abbiamo almeno 5 modelli coinvolti ( Submission , Charge , ChargeType , CcTransaction e CreditCard ). Se dovessi farlo da zero, userei solo un singolo modello Payment . Ci sono solo 2 tipi di addebito, quindi vorrei codificare questi valori come variabili di classe. Non memorizziamo i dettagli della carta di credito, quindi il modello non è necessario. Le informazioni sulla transazione possono essere memorizzate nella tabella payments . I pagamenti non riusciti non devono essere salvati.
Potrei entrare e fare questo refactoring abbastanza facilmente tranne che per il requisito che nulla dovrebbe mai andare storto sul server di produzione. Ciascuna delle classi ridondanti ha molti metodi che possono essere chiamati da qualsiasi punto della base di codice. Esiste una suite di test di integrazione ma la copertura non è al 100%.
Come dovrei procedere per refactoring questo mentre assicura che non si rompa nulla? Se passassi attraverso le 5 classi di pagamento e grep ed ogni metodo per scoprire dove sono chiamate, c'è un'alta probabilità di perdere qualcosa. Il client è già utilizzato per l'esecuzione del codice corrente e l'introduzione di nuovi bug non è accettabile. Oltre ad aumentare la copertura del test al 100%, esiste un modo per ridefinirlo con la certezza che nulla si romperà?