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 Submission
modelo:
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 Charge
modelo:
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 user
y encontrar el último en CreditCard
lugar de pasar el que acaba de guardar. Además, este código depende de una ChargeType
carga de la base de datos con una identificación codificada.
En CcTransaction
continuamos 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_transactions
tabla de la base de datos. El proceso de pago real se realiza en el CreditCard
modelo. 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 Payment
modelo. 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 payments
tabla. 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 grep
edité 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á?
fuente
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.Respuestas:
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:
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
Charge
al menos.