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

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?

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 .