Refactorización del código de procesamiento de pagos bizantinos con un presupuesto limitado [cerrado]

8

He estado trabajando en una gran aplicación de Ruby on Rails durante varios años. Fue heredado en un mal estado, pero la mayoría de los errores de producción se han solucionado con el tiempo. Hay algunas secciones que no se han tocado, como el código de procesamiento de pagos. El código funciona en su mayor parte, excepto que cada vez que el procesador de pago niega un cargo, el usuario recibe un error 500 en lugar de un mensaje útil. Me gustaría refactorizar el código para que sea más fácil de mantener. Proporcionaré un breve resumen de cómo funciona.

Eliminé todo el código de manejo de errores de los siguientes fragmentos.

El laberinto comienza en un controlador:

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

Luego en el Submissionmodelo:

  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

En el Chargemodelo:

  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

Hay muchos bits cuestionables arriba, como volver a cargar usery encontrar el último en CreditCardlugar de pasar el que acaba de guardar. Además, este código depende de una ChargeTypecarga de la base de datos con una identificación codificada.

En CcTransactioncontinuamos por el camino:

  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

Todo lo que parece hacer es guardar un registro en la cc_transactionstabla de la base de datos. El proceso de pago real se realiza en el CreditCardmodelo. No te aburriré con los detalles de esa clase. El trabajo real es realizado por ActiveMerchant::Billing::AuthorizeNetCimGateway.

Así que tenemos al menos 5 modelos involucrados ( Submission, Charge, ChargeType, CcTransaction, y CreditCard). Si tuviera que hacer esto desde cero, solo usaría un solo Paymentmodelo. Solo hay 2 tipos de carga, por lo que codificaría esos valores como variables de clase. No almacenamos detalles de la tarjeta de crédito, por lo que ese modelo es innecesario. La información de la transacción se puede almacenar en la paymentstabla. Los pagos fallidos no necesitan guardarse.

Podría entrar y hacer esta refactorización con bastante facilidad, excepto por el requisito de que nada debería salir mal en el servidor de producción. Cada una de las clases redundantes tiene muchos métodos que se pueden invocar desde cualquier lugar de la base del código. Existe un conjunto de pruebas de integración, pero la cobertura no es del 100%.

¿Cómo debo hacer para refactorizar esto y asegurarme de que nada se rompa? Si revisé las 5 clases de pago y grepedité todos los métodos para averiguar dónde se llaman, hay una alta probabilidad de que pierda algo. El cliente ya está acostumbrado a cómo se ejecuta el código actual y la introducción de nuevos errores es inaceptable. Además de aumentar la cobertura de la prueba al 100%, ¿hay alguna forma de refactorizar esto con la certeza de que nada se romperá?

Reed G. Law
fuente
66
Usted ha mencionado solo un aspecto de este código que no funciona: da un error 500 cuando falla el pago. Sin embargo, esto debería ser bastante fácil de solucionar sin rediseñar todo. ¿Hay otras razones concretas por las que este código debe reescribirse? El código feo que funciona generalmente debe dejarse en su lugar, a menos que exista una razón sólida para cambiarlo. Como te preocupa, el rediseño requiere mucho esfuerzo y puede introducir nuevos problemas.
Parecería más fácil arreglar la página 500, pero la dificultad proviene del mal diseño. La página 500 se debe a una AASM::InvalidTransition: Event 'process' cannot transition from 'failed'excepción que enmascara el error real, que es una transacción fallida. Hay tanta indirección que es difícil devolver la respuesta al usuario y permitir una nueva presentación. Estoy seguro de que es posible, pero parece casi tan difícil como refactorizar.
Reed G. Law
2
"y codicié todos los métodos para descubrir dónde se llaman hay una alta probabilidad de que extrañe algo", parece que parte del problema es el hecho de que estás utilizando un lenguaje en el que ningún compilador puede decirte exactamente dónde se llama un método. En tal situación, probablemente deba resistir el deseo de refactorizar, cuánto sentido tendrá alguna vez.
Doc Brown

Respuestas:

20

Considere si este código realmente necesita refactorización . El código puede ser desagradable y estéticamente desagradable para usted como desarrollador de software, pero si funciona, probablemente no debería rediseñarlo. Y según su pregunta, parece que el código funciona en gran medida.

Este artículo clásico de Joel sobre software destaca todos los riesgos de reescribir código innecesariamente. Es un error costoso, pero muy tentador. Vale la pena leer todo antes, pero este pasaje sobre una complejidad aparentemente innecesaria parece particularmente apropiado:

De vuelta a esa función de dos páginas. Sí, lo sé, es solo una función simple para mostrar una ventana, pero le han crecido pequeños pelos y cosas y nadie sabe por qué. Bueno, te diré por qué: esas son correcciones de errores. Uno de ellos corrige ese error que tenía Nancy cuando intentaba instalarlo en una computadora que no tenía Internet Explorer. Otro corrige ese error que ocurre en condiciones de poca memoria. Otro corrige ese error que ocurrió cuando el archivo está en un disquete y el usuario saca el disco en el medio. Esa llamada de LoadLibrary es fea, pero hace que el código funcione en versiones antiguas de Windows 95.

Cada uno de estos errores tomó semanas de uso en el mundo real antes de ser encontrados. El programador podría haber pasado un par de días reproduciendo el error en el laboratorio y reparándolo. Si se trata de muchos errores, la solución podría ser una línea de código, o incluso podría ser un par de caracteres, pero se dedicó mucho trabajo y tiempo a esos dos caracteres.

Sí, el código es innecesariamente complejo. Pero aún así, algunos de los pasos que parecen innecesarios pueden estar allí por una razón. Y si vas a reescribirlo, tendrás que aprender todas esas lecciones nuevamente.

Recargar al usuario, por ejemplo, parece inútil: es exactamente por eso que debe preocuparse por cambiarlo. Ningún desarrollador (incluso uno malo) habría introducido un paso como ese en su diseño inicial. Es casi seguro que se puso allí para solucionar algún problema. Tal vez sea un problema que la refactorización al diseño "correcto" eliminará ... pero tal vez no.

Además, otro pequeño punto, no estoy convencido por su afirmación de que no es necesario guardar los pagos fallidos. Puedo pensar en dos razones importantes para hacerlo: registrar evidencia de posible fraude (por ejemplo, alguien que prueba varios números de tarjeta) y atención al cliente (un cliente afirma que sigue intentando pagar, y no está funcionando ... desearía evidencia de cualquier intento de pago para ayudar a solucionar este problema). Esto me hace pensar que quizás no has pensado completamente en los requisitos del sistema, y ​​que no son tan simples como crees.

Solucione problemas individuales sin cambiar fundamentalmente el diseño. Mencionó un problema: hay un error 500 cuando el pago falla. Este problema debería ser fácil de solucionar, probablemente simplemente agregando una o dos líneas en la ubicación correcta. No es razón suficiente para destrozar todo para hacer el diseño "correcto".

Puede haber otros problemas, pero si el código funciona el 99% del tiempo en este momento, es poco probable que estos sean problemas fundamentales que requieran una reescritura.

Si el diseño actual está incrustado en todo el código, puede estar justificado gastar una buena cantidad de esfuerzo para solucionar un problema sin cambiar el diseño.

En algunos casos, de hecho, puede ser necesario hacer un rediseño más grande. Pero esto requeriría una justificación más sólida que ha dado hasta ahora. Por ejemplo, si planea desarrollar aún más el modelo de pago e introducir nuevas características significativas, limpiar el código tendría algún beneficio. O quizás el diseño contiene una falla de seguridad fundamental que necesita ser reparada.

Hay razones para refactorizar una gran porción de código. Pero si lo hace, aumente la cobertura de la prueba al 100%. Esto es probablemente algo que le ahorrará tiempo en general .


fuente
2
La cita del artículo de Joel no se aplica en este caso porque conozco los casos de uso actuales y que gran parte del código es innecesario. En cuanto a aumentar la cobertura de prueba al 100%, pasé muchos meses obteniendo cobertura de hasta aproximadamente el 80%. El código descubierto restante es menos crítico, más difícil de probar o ambos. Incluso con una cobertura de prueba del 100% informada, no confiaría en las pruebas automatizadas para detectar todos los casos posibles a menos que pasara un tiempo de magnitud más tiempo escribiendo pruebas. Otra racional para la refactorización es que cada vez que se visita esta sección, lleva mucho tiempo entenderla.
Reed G. Law
3
@ ReedG.Law, otra posibilidad es simplificar la lógica internamente, pero no cambiar la interfaz que está expuesta al resto del código, una especie de rediseño a mitad de camino que es de menor riesgo. Sin embargo, esto aún conllevaría cierto riesgo de introducir nuevos problemas.
esa es una posibilidad, pero hay una gran área de superficie en la interfaz debido al código estrechamente acoplado. Podría ser capaz de deshacerme de la máquina de estado Chargeal menos.
Reed G. Law
2
@ ReedG.Law, aparte, me sorprende que este código esté integrado en muchos lugares diferentes del sitio. La mayoría de los sitios de comercio electrónico tienen una única ruta de "pago" definida, y solo esperaría que se llamara al código de pago en esa ubicación. Si ese no es el caso, ¿tal vez el código de llamada es realmente lo que necesita atención, antes de este código? Si realmente necesita rediseñar, tal vez el proceso sea: 1) asegúrese de que todo el sitio se dirija a una única ruta para el pago. 2) después de haberlo hecho, puede rediseñar de manera segura el código de pago.
3
@ ReedG.Law: dices porque conozco los casos de uso actuales , pero tu publicación original indica que cada una de las clases redundantes tiene muchos métodos a los que se puede llamar desde cualquier lugar de la base del código . Esas 2 citas parecen ser mutuamente excluyentes.
Kickstart