Refactoring des byzantinischen Zahlungsverarbeitungscodes mit begrenztem Budget [geschlossen]

8

Ich arbeite seit mehreren Jahren an einer großen Ruby on Rails-Anwendung. Es wurde in einem schlechten Zustand vererbt, aber die meisten Produktionsfehler wurden mit der Zeit behoben. Es gibt einige Abschnitte, die nicht berührt wurden, z. B. den Zahlungsverarbeitungscode. Der Code funktioniert größtenteils, außer dass der Benutzer immer dann, wenn eine Gebühr vom Zahlungsprozessor abgelehnt wird, einen 500-Fehler anstelle einer hilfreichen Nachricht erhält. Ich möchte den Code überarbeiten, um die Wartung zu vereinfachen. Ich werde einen kurzen Überblick darüber geben, wie es funktioniert.

Ich habe den gesamten Fehlerbehandlungscode aus den folgenden Ausschnitten entfernt.

Das Labyrinth beginnt in einem Controller:

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

Dann im SubmissionModell:

  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

Im ChargeModell:

  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

Es gibt viele fragwürdige Punkte oben, wie das Nachladen des userund das Finden des letzten, CreditCardanstatt das gerade gespeicherte weiterzugeben. Auch dieser Code hängt von einer ChargeTypeaus der Datenbank geladenen mit einer fest codierten ID ab.

In gehen CcTransactionwir weiter den Weg hinunter:

  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

Dies scheint lediglich einen Datensatz in der cc_transactionsDatenbanktabelle zu speichern. Die eigentliche Zahlungsabwicklung erfolgt im CreditCardModell. Ich werde Sie nicht mit den Details dieser Klasse langweilen. Die eigentliche Arbeit wird von ausgeführt ActiveMerchant::Billing::AuthorizeNetCimGateway.

Also haben wir mindestens fünf Modelle haben beteiligt sind ( Submission, Charge, ChargeType, CcTransaction, und CreditCard). Wenn ich das von Grund auf neu machen würde, würde ich nur ein einziges PaymentModell verwenden. Es gibt nur zwei Gebührentypen, daher würde ich diese Werte als Klassenvariablen fest codieren. Wir speichern keine Kreditkartendaten, sodass dieses Modell nicht erforderlich ist. Transaktionsinformationen können in der paymentsTabelle gespeichert werden. Fehlgeschlagene Zahlungen müssen nicht gespeichert werden.

Ich könnte dieses Refactoring ziemlich einfach durchführen, außer der Anforderung, dass auf dem Produktionsserver niemals etwas schief gehen sollte. Jede der redundanten Klassen verfügt über viele Methoden, die von überall in der Codebasis aufgerufen werden können. Es gibt eine Reihe von Integrationstests, aber die Abdeckung beträgt nicht 100%.

Wie soll ich vorgehen, um sicherzustellen, dass nichts kaputt geht? Wenn ich die 5 Zahlungsklassen grepdurchgesehen und jede Methode bearbeitet habe, um herauszufinden, wo sie heißen, besteht eine hohe Wahrscheinlichkeit, dass ich etwas verpasse. Der Client ist bereits daran gewöhnt, wie der aktuelle Code ausgeführt wird, und das Einführen neuer Fehler ist nicht akzeptabel. Gibt es eine Möglichkeit, die Testabdeckung auf 100% zu erhöhen, um sicher zu sein, dass nichts kaputt geht?

Reed G. Law
quelle
6
Sie haben nur einen Aspekt dieses Codes erwähnt, der fehlerhaft ist: Es gibt einen 500-Fehler, wenn die Zahlung fehlschlägt. Dies sollte jedoch ziemlich einfach zu beheben sein, ohne das Ganze neu zu gestalten. Gibt es andere konkrete Gründe, warum dieser Code neu geschrieben werden muss? Hässlicher Code, der funktioniert, sollte normalerweise belassen werden, es sei denn, es gibt eine starke Begründung für die Änderung. Wie Sie zu Recht befürchten, ist die Neugestaltung sehr aufwändig und kann zu neuen Problemen führen.
Es scheint einfacher zu sein, die 500 Seiten zu reparieren, aber die Schwierigkeit ergibt sich aus dem schlechten Design. Die 500-Seite ist auf eine AASM::InvalidTransition: Event 'process' cannot transition from 'failed'Ausnahme zurückzuführen, die den tatsächlichen Fehler maskiert, bei dem es sich um eine nicht erfolgreiche Transaktion handelt. Es gibt so viele Indirektionen, dass es schwierig ist, die Antwort an den Benutzer zurückzusenden und eine erneute Übermittlung zuzulassen. Ich bin mir sicher, dass es möglich ist, aber es scheint fast so schwierig wie Refactoring.
Reed G. Law
2
"und jede Methode durchsucht, um herauszufinden, wo sie aufgerufen wird, besteht eine hohe Wahrscheinlichkeit, dass ich etwas verpasse" - ein Teil des Problems ist die Tatsache, dass Sie eine Sprache verwenden, in der kein Compiler Ihnen genau sagen kann, wo eine Methode aufgerufen wird. In einer solchen Situation müssen Sie sich wahrscheinlich dem Wunsch widersetzen, umzugestalten, wie viel Sinn es jemals machen wird.
Doc Brown

Antworten:

20

Überlegen Sie, ob dieser Code wirklich überarbeitet werden muss . Der Code mag für Sie als Softwareentwickler hässlich und ästhetisch unangenehm sein, aber wenn er funktioniert, sollten Sie ihn wahrscheinlich nicht neu gestalten. Und basierend auf Ihrer Frage klingt es so, als ob der Code weitgehend funktioniert.

Dieser klassische Artikel von Joel on Software zeigt alle Risiken eines unnötigen Umschreibens von Code auf. Es ist ein kostspieliger, aber sehr verlockender Fehler. Das Ganze ist schon einmal lesenswert, aber diese Passage über scheinbar unnötige Komplexität erscheint besonders angebracht:

Zurück zu dieser Doppelseitenfunktion. Ja, ich weiß, es ist nur eine einfache Funktion, ein Fenster anzuzeigen, aber es hat kleine Haare und Sachen darauf und niemand weiß warum. Nun, ich sage Ihnen warum: Das sind Fehlerbehebungen. Einer von ihnen behebt den Fehler, den Nancy hatte, als sie versuchte, das Ding auf einem Computer ohne Internet Explorer zu installieren. Ein anderer behebt den Fehler, der unter Bedingungen mit wenig Speicher auftritt. Ein anderer behebt den Fehler, der aufgetreten ist, als sich die Datei auf einer Diskette befindet und der Benutzer die Diskette in der Mitte herauszieht. Dieser LoadLibrary-Aufruf ist hässlich, aber der Code funktioniert unter alten Versionen von Windows 95.

Jeder dieser Fehler dauerte Wochen, bis er gefunden wurde. Der Programmierer hat möglicherweise einige Tage damit verbracht, den Fehler im Labor zu reproduzieren und zu beheben. Wenn es sich um eine Menge Fehler handelt, kann es sich bei dem Fix um eine Codezeile oder sogar um ein paar Zeichen handeln, aber diese beiden Zeichen haben viel Arbeit und Zeit aufgewendet.

Ja, der Code ist unnötig komplex. Dennoch können einige der Schritte, die unnötig erscheinen, aus einem bestimmten Grund vorhanden sein. Und wenn Sie es neu schreiben, müssen Sie all diese Lektionen noch einmal lernen.

Das Neuladen des Benutzers zum Beispiel erscheint sinnlos: Genau deshalb sollten Sie sich Gedanken über das Ändern machen. Kein Entwickler (auch kein schlechter) hätte einen solchen Schritt in sein ursprüngliches Design eingeführt. Es wurde mit ziemlicher Sicherheit dort eingesetzt, um ein Problem zu beheben. Vielleicht ist es ein Problem, das durch die Umgestaltung auf das "richtige" Design beseitigt wird ... aber vielleicht auch nicht.

Ein weiterer kleiner Punkt, ich bin nicht von Ihrer Behauptung überzeugt, dass es nicht notwendig ist, fehlgeschlagene Zahlungen zu speichern. Ich kann mir zwei starke Gründe dafür vorstellen: Protokollierung von Beweisen für möglichen Betrug (z. B. jemanden, der verschiedene Kartennummern ausprobiert) und Kundenunterstützung (ein Kunde behauptet, er versuche weiterhin zu zahlen, und es funktioniert nicht ... Sie möchten Beweise von versuchten Zahlungsversuchen, um dies zu beheben). Dies lässt mich denken, dass Sie die Anforderungen des Systems vielleicht nicht vollständig durchdacht haben und sie nicht so einfach sind, wie Sie glauben.

Beheben Sie individuelle Probleme, ohne das Design grundlegend zu ändern. Sie haben ein Problem erwähnt: Es gibt einen 500-Fehler, wenn die Zahlung fehlschlägt. Dieses Problem sollte leicht zu beheben sein, wahrscheinlich nur durch Hinzufügen von ein oder zwei Zeilen an der richtigen Stelle. Es reicht nicht aus, alles auseinander zu reißen, um das "richtige" Design zu erhalten.

Möglicherweise gibt es andere Probleme, aber wenn der Code derzeit in 99% der Fälle funktioniert, ist es unwahrscheinlich, dass es sich um grundlegende Probleme handelt, die ebenfalls umgeschrieben werden müssen.

Wenn das aktuelle Design im gesamten Code eingebettet ist, kann es gerechtfertigt sein, einen angemessenen Aufwand zu betreiben, um ein Problem zu beheben, ohne das Design zu ändern.

In einigen Fällen kann es tatsächlich erforderlich sein, eine größere Neugestaltung vorzunehmen. Dies würde jedoch eine stärkere Begründung erfordern, die Sie bisher gegeben haben. Wenn Sie beispielsweise vorhaben, das Zahlungsmodell weiterzuentwickeln und wichtige neue Funktionen einzuführen, hat die Bereinigung des Codes einige Vorteile. Oder vielleicht enthält das Design eine grundlegende Sicherheitslücke, die behoben werden muss.

Es gibt Gründe, einen großen Teil des Codes umzugestalten. Wenn Sie dies tun, erhöhen Sie die Testabdeckung auf 100%. Dies ist wahrscheinlich etwas, das Ihnen insgesamt Zeit spart .


quelle
2
Das Zitat aus Joels Artikel trifft in diesem Fall nicht zu, da ich die aktuellen Anwendungsfälle kenne und ein Großteil des Codes in der Tat unnötig ist. Um die Testabdeckung auf 100% zu erhöhen, habe ich viele Monate damit verbracht, die Abdeckung auf etwa 80% zu erhöhen. Der verbleibende nicht abgedeckte Code ist entweder weniger kritisch, schwerer zu testen oder beides. Selbst bei einer gemeldeten 100% igen Testabdeckung würde ich nicht darauf vertrauen, dass automatisierte Tests jeden möglichen Randfall erfassen, es sei denn, ich habe eine Größenordnung mehr Zeit damit verbracht, Tests zu schreiben. Ein weiterer Grund für das Refactoring ist, dass jedes Mal, wenn dieser Abschnitt besucht wird, das Verständnis lange dauert.
Reed G. Law
3
@ ReedG.Law, eine weitere Möglichkeit besteht darin, die Logik intern zu vereinfachen, aber nicht die Schnittstelle zu ändern, die dem Rest des Codes ausgesetzt ist - eine Art halbwegs neues Redesign mit geringerem Risiko. Dies birgt jedoch immer noch das Risiko, neue Probleme einzuführen.
Das ist eine Möglichkeit, aber die Schnittstelle hat aufgrund des eng gekoppelten Codes eine große Oberfläche. Ich könnte zumindest in der Lage sein, die Zustandsmaschine loszuwerden Charge.
Reed G. Law
2
@ ReedG.Law beiseite, ich bin überrascht, dass dieser Code viele verschiedene Stellen in der Website eingebettet werden würde. Die meisten E-Commerce-Websites haben einen einzigen definierten "Checkout" -Pfad, und ich würde nur erwarten, dass der Zahlungscode an diesem einen Ort aufgerufen wird. Wenn dies nicht der Fall ist, ist der aufrufende Code möglicherweise wirklich das, was vor diesem Code beachtet werden muss? Wenn Sie wirklich neu gestalten müssen, ist der Prozess möglicherweise: 1) Stellen Sie sicher, dass die gesamte Site zum Auschecken auf einen einzelnen Pfad verweist. 2) Nachdem Sie dies getan haben, können Sie den Zahlungscode sicher neu gestalten.
3
@ ReedG.Law - Sie sagen, weil ich die aktuellen Anwendungsfälle kenne , aber Ihr ursprünglicher Beitrag besagt, dass jede der redundanten Klassen viele Methoden hat, die von überall in der Codebasis aufgerufen werden können . Diese beiden Anführungszeichen scheinen sich gegenseitig auszuschließen.
Kickstart