Refatorando código de processamento de pagamento bizantino com um orçamento limitado [fechado]

8

Estou trabalhando em um grande aplicativo Ruby on Rails há vários anos. Foi herdado em um estado ruim, mas a maioria dos erros de produção foi resolvida com o tempo. Existem algumas seções que não foram tocadas, como o código de processamento de pagamentos. O código funciona na maior parte do tempo, exceto que sempre que uma cobrança é negada pelo processador de pagamento, o usuário recebe um erro 500 em vez de uma mensagem útil. Gostaria de refatorar o código para facilitar a manutenção. Vou fornecer uma breve descrição de como isso funciona.

Removi todo o código de tratamento de erros dos seguintes trechos.

O labirinto começa em um controlador:

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

Depois, no 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

No 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

Existem muitos bits questionáveis ​​acima, como recarregar usere encontrar o último, em CreditCardvez de passar o que acabou de salvar. Além disso, esse código depende de um ChargeTypecarregamento do banco de dados com um ID codificado.

Em CcTransactioncontinuamos a trilha:

  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

Tudo isso parece fazer é salvar um registro na cc_transactionstabela do banco de dados. O processamento de pagamento real é realizado no CreditCardmodelo. Não vou aborrecê-lo com os detalhes dessa aula. O trabalho real é realizado por ActiveMerchant::Billing::AuthorizeNetCimGateway.

Portanto, temos pelo menos 5 modelos envolvidos ( Submission, Charge, ChargeType, CcTransaction, e CreditCard). Se eu fizesse isso do zero, usaria apenas um único Paymentmodelo. Existem apenas dois tipos de cobrança, então eu codificaria esses valores como variáveis ​​de classe. Não armazenamos detalhes do cartão de crédito, portanto esse modelo é desnecessário. As informações da transação podem ser armazenadas na paymentstabela. Os pagamentos com falha não precisam ser salvos.

Eu poderia entrar e fazer essa refatoração com bastante facilidade, exceto pelo requisito de que nada nunca desse errado no servidor de produção. Cada uma das classes redundantes possui muitos métodos que podem ser chamados de qualquer lugar na base de código. Há um conjunto de testes de integração, mas a cobertura não é 100%.

Como devo refatorar isso, garantindo que nada se quebre? Se eu passasse pelas 5 classes de pagamento e grepeditasse todos os métodos para descobrir como eles são chamados, há uma alta probabilidade de que eu perca alguma coisa. O cliente já está acostumado a como o código atual é executado e a introdução de novos bugs é inaceitável. Além de aumentar a cobertura de teste para 100%, existe alguma maneira de refatorar isso com certeza de que nada vai quebrar?

Reed G. Law
fonte
6
Você mencionou apenas um aspecto desse código que está quebrado: ele gera um erro 500 quando o pagamento falha. Isso deve ser bastante fácil de corrigir, sem redesenhar a coisa toda. Existem outras razões concretas pelas quais esse código precisa ser reescrito? O código feio que funciona normalmente deve ser deixado, a menos que haja uma forte justificativa para alterá-lo. Como você se preocupa, o redesenho exige muito esforço e pode apresentar novos problemas.
Parece mais fácil corrigir a página 500, mas a dificuldade decorre do design inadequado. A página 500 é devido a uma AASM::InvalidTransition: Event 'process' cannot transition from 'failed'exceção que mascara o erro real, que é uma transação malsucedida. Há tanta indireção que é difícil obter a resposta de volta ao usuário e permitir um reenvio. Tenho certeza de que é possível, mas parece quase tão difícil quanto a refatoração.
Reed G. Lei
2
"e selecionamos todos os métodos para descobrir onde eles são chamados, existe uma alta probabilidade de que eu sinta falta de algo" - parece que parte do problema é o fato de você estar usando uma linguagem em que nenhum compilador pode dizer exatamente onde um método é chamado. Em tal situação, você provavelmente terá que resistir ao desejo de refatorar, quanto sentido isso fará.
Doc Brown

Respostas:

20

Considere se esse código realmente precisa de refatoração . O código pode ser feio e esteticamente desagradável para você como desenvolvedor de software, mas, se funcionar, você provavelmente não deve reprojetá-lo. E, com base na sua pergunta, parece que o código funciona amplamente.

Este artigo clássico de Joel on Software destaca todos os riscos de reescrever códigos desnecessariamente. É um erro caro, mas muito tentador. Vale a pena ler tudo antes, mas essa passagem sobre complexidade aparentemente desnecessária parece particularmente apropriada:

Voltar para a função de duas páginas. Sim, eu sei, é apenas uma função simples exibir uma janela, mas ela tem poucos pêlos e outras coisas e ninguém sabe o porquê. Bem, vou lhe dizer o porquê: essas são correções de bugs. Um deles corrige o bug que Nancy tinha quando tentou instalar o dispositivo em um computador que não tinha o Internet Explorer. Outro corrige o bug que ocorre em condições de pouca memória. Outro corrige o bug que ocorreu quando o arquivo está em um disquete e o usuário arranca o disco no meio. Essa chamada LoadLibrary é feia, mas faz o código funcionar em versões antigas do Windows 95.

Cada um desses erros levou semanas de uso no mundo real antes de serem encontrados. O programador pode ter passado alguns dias reproduzindo o bug no laboratório e corrigindo-o. Se houver muitos bugs, a correção pode ser uma linha de código ou até dois caracteres, mas muito trabalho e tempo foram usados ​​nesses dois caracteres.

Sim, o código é desnecessariamente complexo. Mas, ainda assim, algumas das etapas que parecem desnecessárias podem estar lá por um motivo. E se você reescrevê-lo, terá que aprender todas essas lições novamente.

Recarregar o usuário, por exemplo, parece inútil: e é exatamente por isso que você deve se preocupar em alterá-lo. Nenhum desenvolvedor (mesmo um ruim) teria introduzido uma etapa como essa em seu design inicial. Foi quase certamente colocado lá para corrigir algum problema. Talvez seja um problema que a refatoração para o design "correto" elimine ... mas talvez não.

Além disso, outro pequeno ponto, não estou convencido de sua afirmação de que não é necessário salvar os pagamentos com falha. Posso pensar em duas razões fortes para fazer isso: registrar evidências de possíveis fraudes (por exemplo, alguém tentando vários números de cartões) e suporte ao cliente (um cliente alega que eles continuam tentando pagar e não está funcionando ... você deseja obter evidências de qualquer tentativa de pagamento para ajudar a solucionar isso). Isso me faz pensar que talvez você não tenha pensado completamente nos requisitos do sistema, e eles não são tão simples quanto você acredita.

Corrija problemas individuais sem alterar fundamentalmente o design. Você mencionou um problema: há um erro 500 quando o pagamento falha. Esse problema deve ser fácil de corrigir, provavelmente apenas adicionando uma ou duas linhas no local certo. Não é suficiente justificativa para separar tudo para criar o design "correto".

Pode haver outros problemas, mas se o código funcionar 99% do tempo no momento, é improvável que esses sejam problemas fundamentais que exigem uma reescrita.

Se o design atual estiver incorporado ao longo do código, pode ser necessário até um esforço considerável para corrigir um problema sem alterar o design.

Em alguns casos, pode ser realmente necessário fazer um redesenho maior. Mas isso exigiria uma lógica mais forte que você deu até agora. Por exemplo, se você planeja desenvolver ainda mais o modelo de pagamento e introduzir novos recursos significativos, a limpeza do código traria algum benefício. Ou talvez o design contenha uma falha de segurança fundamental que precisa ser corrigida.

Existem razões para refatorar um grande pedaço de código. Mas se você fizer isso, aumente a cobertura do teste para 100%. Provavelmente, isso é algo que economizará seu tempo .


fonte
2
A citação do artigo de Joel não se aplica neste caso, porque conheço os casos de uso atuais e que muito do código é realmente desnecessário. Quanto a aumentar a cobertura de teste para 100%, passei muitos meses obtendo cobertura de até 80%. O código descoberto restante é menos crítico, mais difícil de testar ou ambos. Mesmo com 100% de cobertura de teste relatada, eu não confiava em testes automatizados para detectar todos os casos extremos possíveis, a menos que eu passasse uma ordem de magnitude mais tempo escrevendo testes. Outro racional para a refatoração é que toda vez que esta seção é visitada, leva muito tempo para entender.
Reed G. Lei
3
@ ReedG.Law, uma outra possibilidade é simplificar a lógica internamente, mas não alterar a interface que é exposta ao restante do código - uma espécie de redesenho a meio caminho que apresenta menor risco. Isso ainda correria o risco de introduzir novos problemas.
essa é uma possibilidade, mas há uma grande área de superfície na interface devido ao código fortemente acoplado. Talvez eu consiga me livrar da máquina de estado Chargepelo menos.
Reed G. Lei
2
@ ReedG.Law como um aparte, estou surpreso que esse código seja incorporado em muitos lugares diferentes no site. A maioria dos sites de comércio eletrônico tem um único caminho definido de "checkout", e eu esperaria apenas que o código de pagamento fosse chamado nesse local. Se não for esse o caso, talvez o código de chamada seja realmente o que precisa de atenção antes desse código? Se você realmente precisar reprojetar, talvez o processo seja: 1) verifique se o site inteiro direciona para um único caminho para checkout. 2) depois de fazer isso, você pode redesenhar com segurança o código de pagamento.
3
@ ReedG.Law - você diz, porque eu conheço os casos de uso atuais , mas sua postagem original declara. Cada uma das classes redundantes tem muitos métodos que podem ser chamados de qualquer lugar na base de código . Essas duas citações parecem ser mutuamente exclusivas.
Kickstart #