Refactoring des byzantinischen Zahlungsverarbeitungscodes mit begrenztem Budget [geschlossen]

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?

Ü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 .