Devo refatorar o código marcado como "não mude"?

148

Estou lidando com uma grande base de código e recebi alguns meses para refatorar o código existente. O processo de refatoração é necessário, pois em breve precisaremos adicionar muitos novos recursos ao nosso produto e, por enquanto, não podemos mais adicionar nenhum recurso sem interromper outra coisa. Em resumo: código bagunçado, enorme e com erros, que muitos de nós já vimos em suas carreiras.

Durante a refatoração, de tempos em tempos eu encontro a classe, método ou linhas de código que têm comentários como

Tempo limite definido para dar ao Módulo A algum tempo para fazer as coisas. Se não for cronometrado como este, ele será interrompido.

ou

Não mude isso. Confie em mim, você vai quebrar as coisas.

ou

Sei que usar setTimeout não é uma boa prática, mas nesse caso eu tive que usá-lo

Minha pergunta é: devo refatorar o código quando encontrar tais avisos dos autores (não, não consigo entrar em contato com os autores)?

kukis
fonte
157
Parabéns! Agora você é o autor do código.
12
Você não pode corrigi-lo até saber o que ele deve fazer e o que ele realmente faz (você precisa conhecê-lo para entender todas as consequências de suas alterações.) O problema parece estar na sincronização e na remoção de tais problemas. deve refatorar o trabalho 1, ou as coisas pioram a cada mudança. A pergunta que você deve fazer é como fazer isso. Existe uma boa quantidade de dependência de idioma e plataforma nessa questão. por exemplo: stackoverflow.com/questions/3099794/finding-threading-bugs
sdenham
14
Seguindo o comentário de @ sdenham: se você ainda não possui testes de unidade automatizados que exercitam sua base de código, sugiro que você gaste seu tempo criando esses testes de unidade, em vez de perder tempo em uma "refatoração de caça às bruxas" com AFAICT sem objetivos claros em mente. Depois de ter um conjunto de testes de unidade que exercitam completamente sua base de código, você terá uma maneira objetiva de saber se o seu código ainda funciona se / quando você precisar reescrever partes dele. Boa sorte.
Bob Jarvis
17
Se os autores são tão paranóicos que o código irá quebrar se alguém respirar perto dele, provavelmente já está quebrado e o comportamento indefinido acaba por funcionar da maneira que eles querem no momento. O primeiro, em particular, parece que eles têm uma condição de corrida que eles julgaram, então isso meio que funciona na maioria das vezes. Sdenham está certo. Descubra o que deve fazer e não assuma que é o que realmente faz.
Raio
7
@ Ethan Pode não ser tão simples assim. Alterar o código pode quebrá-lo de maneiras que não são imediatamente aparentes. Às vezes, ele quebra muito depois que você esquece que esse refator atual poderia ter causado isso, então tudo o que você tem é um "novo" bug com causa misteriosa. Isso é especialmente provável quando relacionado ao tempo.
Paul Brinkley

Respostas:

225

Parece que você está refatorando "por precaução", sem saber exatamente quais partes da base de código em detalhes serão alteradas quando o desenvolvimento do novo recurso ocorrer. Caso contrário, você saberia se há uma necessidade real de refatorar os módulos quebradiços ou se pode deixá-los como estão.

Para dizer a verdade: acho que essa é uma estratégia de refatoração condenada . Você está investindo tempo e dinheiro da sua empresa em algo em que ninguém sabe se isso realmente trará um benefício, e você está prestes a piorar as coisas introduzindo bugs no código de trabalho.

Aqui está uma estratégia melhor: use seu tempo para

  • adicione testes automáticos (provavelmente não testes de unidade, mas testes de integração) aos módulos em risco. Especialmente os módulos quebradiços que você mencionou precisarão de um conjunto de testes completo antes de alterar qualquer coisa.

  • refatorar apenas os bits necessários para implementar os testes. Tente minimizar qualquer uma das alterações necessárias. A única exceção é quando seus testes revelam bugs - então conserte-os imediatamente (e refatorar na medida em que você precisar fazer isso com segurança).

  • ensine a seus colegas o "princípio escoteiro" (AKA "refatoração oportunista" ); portanto, quando a equipe começar a adicionar novos recursos (ou corrigir bugs), eles deverão melhorar e refatorar exatamente as partes da base de código que precisam mudar, nem menos, nem mais.

  • obtenha uma cópia do livro de Feather "Trabalhando efetivamente com o código legado" para a equipe.

Portanto, quando chegar a hora de saber com certeza que precisa alterar e refatorar os módulos quebradiços (devido ao desenvolvimento de novos recursos ou porque os testes adicionados na etapa 1 revelam alguns erros), você e sua equipe estão prontos para refatorar os módulos e ignorar com mais ou menos segurança esses comentários de aviso.

Como resposta a alguns comentários : para ser justo, se alguém suspeitar que um módulo de um produto existente é a causa de problemas regularmente, especialmente um módulo marcado como "não toque", concordo com todos os você. Ele deve ser revisado, depurado e provavelmente refatorado nesse processo (com o suporte dos testes mencionados, e não necessariamente nessa ordem). Os erros são uma forte justificativa para a mudança, geralmente mais forte que os novos recursos. No entanto, essa é uma decisão caso a caso. É preciso verificar com muito cuidado se realmente vale a pena alterar algo em um módulo que foi marcado como "não toque".

Doc Brown
fonte
70
Estou tentado a votar, mas vou me abster. Vi essa mentalidade em vários projetos levar a um produto muito pior. Quando as falhas finalmente acontecem, elas geralmente são catastróficas e muito mais difíceis de corrigir devido ao seu entrincheiramento. Corrijo regularmente o lodo que vejo e parece causar, na pior das hipóteses, tantos problemas quanto ele corrige ou, na melhor das hipóteses, não deixa problemas conhecidos; No geral, é uma enorme vantagem. E o código é melhor para o desenvolvimento futuro. Um ponto no tempo economiza nove, mas um problema ignorado pode piorar.
Aaron #
98
Eu diria que qualquer código marcado com um comentário "Tempo limite definido para dar ao Módulo A algum tempo para fazer as coisas. Se não tiver esse tempo programado, ele será interrompido" possui um bug. É apenas sorte do empate que o bug não está sendo atingido.
17 de 26
10
@ 17of26: não necessariamente um bug. Às vezes, quando você trabalha com bibliotecas de terceiros, é forçado a fazer todos os tipos de concessões de "elegância" no objetivo de fazê-lo funcionar.
Whatsisname
30
@ 17of26: se houver suspeita de um módulo em jogo com bugs, adicionar testes antes de qualquer tipo de refatoração é ainda mais importante. E quando esses testes revelam o bug, você precisa alterar esse módulo e, portanto, uma legitimação para refatorá-lo imediatamente. Se os testes não revelarem nenhum erro, é melhor deixar o código como está.
Doc Brown
17
@ Aaron: Eu não entendo por que você acha que adicionar testes ou seguir o princípio dos escoteiros consequentemente reduziria a qualidade do produto.
Doc Brown
142

Sim, você deve refatorar o código antes de adicionar os outros recursos.

O problema com comentários como esses é que eles dependem de circunstâncias específicas do ambiente em que a base de código está sendo executada. O tempo limite programado em um ponto específico pode ter sido realmente necessário quando foi programado.

Mas há várias coisas que podem mudar essa equação: alterações de hardware, alterações do SO, alterações não relacionadas em outro componente do sistema, alterações nos volumes de dados típicos, o que você escolher. Não há garantia de que, nos dias atuais, isso ainda seja necessário ou que ainda seja suficiente (a integridade da coisa que deveria proteger pode ter sido quebrada por um longo tempo - sem testes de regressão adequados, você nunca notará). É por isso que programar um atraso fixo para permitir que outro componente termine quase sempre está incorreto e funciona apenas acidentalmente.

No seu caso, você não conhece as circunstâncias originais e também não pode perguntar aos autores originais. (Presumivelmente, você também não possui testes de regressão / integração adequados, ou pode simplesmente prosseguir e permitir que seus testes informem se você quebrou alguma coisa.)

Isso pode parecer um argumento para não mudar nada por precaução; mas você diz que terá que haver grandes mudanças de qualquer maneira, então o perigo de perturbar o delicado equilíbrio que costumava ser alcançado neste local já está lá. É muito melhor perturbar o carrinho de maçã agora , quando a única coisa que você está fazendo é a refatoração, e esteja certo de que, se as coisas quebrarem, foi a refatoração que o causou, do que esperar até que você esteja fazendo alterações adicionais simultaneamente e nunca tenha certeza.

Kilian Foth
fonte
46
Melhor resposta aqui; Pena que é um azarão. A resposta aceita acima não é um bom conselho, especialmente a ênfase em "condenado". Passei o último ano trabalhando no maior (~ 10 ^ 6LOC apenas para a minha seção), o código mais antigo e horrível que já vi, e tenho o hábito de consertar os destroços de trem que vejo quando tenho o tempo, mesmo que não faça parte do componente em que estou trabalhando. Fazendo isso, encontrei e corrigi bugs que a equipe de desenvolvimento nem sabia que existiam (embora nossos usuários finais provavelmente existissem). Na verdade, eu sou instruído a. O produto foi aprimorado como resultado.
Aaron #
10
Eu não chamaria isso de refatoração, mas correção de bugs, no entanto.
Paŭlo Ebermann
7
Melhorar o design de uma base de código é refatoração. Se o redesenho revelar bugs que podem ser corrigidos, isso é a correção de bugs. O que quero dizer é que o primeiro geralmente leva ao segundo e que o segundo geralmente é muito difícil sem o primeiro.
Kramii
10
@ Aaron, você tem sorte de ter suporte de gerenciamento / desenvolvedor para fazer isso. Na maioria dos ambientes, os bugs conhecidos e desconhecidos causados ​​por design e código ruins são aceitos como a ordem natural das coisas.
Rudolf Olah
5
@nocomprende Eu diria que, se você não entender o sistema bem o suficiente para escrever alguns testes, não terá como mudar o funcionamento dele.
22617 Patrick M #
61

Minha pergunta é: devo refatorar o código quando encontrar tais avisos dos autores

Não, ou pelo menos ainda não. Você implica que o nível de teste automatizado é muito baixo. Você precisa de testes antes de poder refatorar com confiança.

por enquanto não podemos mais adicionar nenhum recurso sem interromper outra coisa

No momento, você precisa se concentrar no aumento da estabilidade, e não na refatoração. Você pode refatorar como parte do aumento da estabilidade, mas é uma ferramenta para atingir seu objetivo real - uma base de código estável.

Parece que isso se tornou uma base de código herdada, então você precisa tratá-la de maneira um pouco diferente.

Comece adicionando testes de caracterização. Não se preocupe com nenhuma especificação, basta adicionar um teste que afirme o comportamento atual. Isso ajudará a impedir que o novo trabalho interrompa os recursos existentes.

Toda vez que você corrigir um bug, adicione um caso de teste que comprove que o bug foi corrigido. Isso evita regressões diretas.

Quando você adiciona um novo recurso, adicione pelo menos alguns testes básicos para que o novo recurso funcione conforme o esperado.

Talvez obtenha uma cópia de "Trabalhando efetivamente com o código herdado"?

Me deram alguns meses para refatorar o código existente.

Comece aumentando a cobertura do teste. Comece com áreas que mais quebram. Comece com áreas que mudam mais. Depois de identificar os designs ruins, substitua-os um por um.

Você quase nunca faz um grande refatorador, mas sim um fluxo constante de pequenos refatores toda semana. Um grande refator tem um enorme risco de quebrar as coisas e é difícil testar bem.

Daenyth
fonte
10
+1 para adicionar testes de caracterização. É praticamente a única maneira de minimizar as regressões ao modificar o código legado não testado. Se os testes começarem a falhar depois que você começar a fazer alterações, poderá verificar se o comportamento anterior estava realmente correto, dada a especificação, ou se o novo comportamento alterado está correto.
Leo
2
Direito. Ou, se não houver especificação útil, verifique se o comportamento anterior é realmente desejado pelas pessoas que pagam ou têm interesse no software.
bdsl
Talvez obtenha uma cópia de "Trabalhando efetivamente com o código herdado"? Quantas semanas levará para ele ler esse livro? E quando: durante o horário de trabalho no local de trabalho, à noite, quando todo mundo dorme?
Billal Begueradj
23

Lembre-se da cerca do GK Chesterton : não derrube uma cerca que obstrua uma estrada até entender por que ela foi construída.

Você pode encontrar o (s) autor (es) do código e os comentários em questão e consultá-los para obter o entendimento. Você pode ver as mensagens de confirmação, os segmentos de email ou os documentos, se existirem. Em seguida, você poderá refatorar o fragmento marcado ou anotará seu conhecimento nos comentários para que a próxima pessoa a manter esse código possa tomar uma decisão mais informada.

Seu objetivo é entender o que o código faz e por que foi marcado com um aviso naquela época e o que aconteceria se o aviso fosse ignorado.

Antes disso, eu não tocava no código marcado como "não toque".

9000
fonte
4
O OP diz "não, não consigo entrar em contato com os autores".
FrustratedWithFormsDesigner
5
Não consigo entrar em contato com os autores nem há documentação.
Kukis #
21
@kukis: Você não pode entrar em contato com os autores, especialistas em domínio e não pode encontrar e-mails / wiki / docs / casos de teste pertencentes ao código em questão? Bem, então é um projeto de pesquisa completo em arqueologia de software, não uma tarefa simples de refatoração.
9000
12
Não é incomum que o código seja antigo e os autores tenham saído. Se eles acabaram trabalhando para um concorrente, discuti-lo pode violar o NDA de alguém. Em alguns casos, os autores estão permanentemente indisponíveis: você não pode perguntar a Phil Katz sobre o PKzip porque ele morreu anos atrás.
Pjc50
2
Se você substituir "encontrar o autor" por "entender qual problema o código resolveu", essa é a melhor resposta. Às vezes, esses bits de código são a solução menos horrível para os bugs que levaram semanas ou meses de trabalho para encontrar e rastrear e lidar com eles, geralmente bugs que tipos normais de teste de unidade não conseguem encontrar. (Embora às vezes eles são o resultado de programadores que não sabem o que estão fazendo sua primeira tarefa é entender o que é.)
grahamparks
6

Código com comentários como os que você mostrou estaria no topo da minha lista de itens a serem refatorados se, e somente se eu tiver algum motivo para fazê-lo . A questão é que o código fede tanto que você até o cheira através dos comentários. Não faz sentido tentar incluir qualquer nova funcionalidade nesse código, esse código precisa morrer assim que for alterado.

Observe também que os comentários não são úteis, no mínimo: eles apenas dão aviso e sem motivo. Sim, são os sinais de alerta em torno de um tanque de tubarões. Mas se você quiser fazer alguma coisa nas proximidades, há alguns pontos para tentar nadar com os tubarões. Imho, você deve se livrar desses tubarões primeiro.

Dito isto, você absolutamente precisa de bons casos de teste antes de se atrever a trabalhar nesse código. Depois de ter esses casos de teste, certifique-se de entender cada pedacinho que estiver mudando, para garantir que você realmente não esteja mudando o comportamento. Torne sua principal prioridade manter todas as peculiaridades comportamentais do código até que você possa provar que elas não têm nenhum efeito. Lembre-se, você está lidando com tubarões - você deve ter muito cuidado.

Portanto, no caso do tempo limite: deixe-o até você entender exatamente o que o código está esperando e, em seguida, corrija o motivo pelo qual o tempo limite foi introduzido antes de continuar com a remoção.

Além disso, certifique-se de que seu chefe entenda que empreendimento você está embarcando e por que é necessário. E se eles disserem não, não faça. Você absolutamente precisa do apoio deles nisso.

cmaster
fonte
1
Às vezes, o código acaba sendo uma bagunça, porque precisa operar corretamente em amostras de pré-produção de silício cujo comportamento real não corresponde à documentação. Dependendo da natureza das soluções alternativas, substituir o código pode ser uma boa ideia, se o código nunca precisar ser executado nos chips de buggy, mas alterar o código sem o nível de análise de engenharia que seria necessário para o novo código provavelmente não é uma boa. idéia.
Supercat
@supercat Um dos meus pontos foi que a refatoração deve preservar perfeitamente o comportamento. Se não preservar o comportamento, é mais do que refatorar no meu livro (e extremamente perigoso ao lidar com esse código legado).
C7
5

Provavelmente não é uma boa idéia refatorar o código com esses avisos, se tudo funcionar bem como está.

Mas se você deve refatorar ...

Primeiro, escreva alguns testes de unidade e testes de integração para testar as condições sobre as quais os avisos o alertam. Tente imitar as condições de produção , tanto quanto possível . Isso significa espelhar o banco de dados de produção para um servidor de teste, executando os mesmos outros serviços na máquina, etc ... o que você puder fazer. Em seguida, tente sua refatoração (em um ramo isolado, é claro). Em seguida, execute seus testes no seu novo código para ver se você pode obter os mesmos resultados que o original. Se você puder, provavelmente não há problema em implementar sua refatoração na produção. Mas esteja pronto para reverter as alterações se tudo der errado.

FrustratedWithFormsDesigner
fonte
5

Eu digo, vá em frente e mude. Acredite ou não, nem todo codificador é um gênio e um aviso como esse significa que poderia ser um local para melhorias. Se você achar que o autor estava certo, poderá (ofegar) DOCUMENT ou EXPLICAR o motivo do aviso.

JES
fonte
4

Estou expandindo meus comentários para uma resposta porque acho que alguns aspectos do problema específico estão sendo ignorados ou usados ​​para tirar conclusões erradas.

Nesse ponto, a questão de refatorar é prematura (mesmo que provavelmente seja respondida por uma forma específica de 'sim').

A questão central aqui é que (conforme observado em algumas respostas) os comentários que você cita indicam fortemente que o código tem condições de corrida ou outros problemas de simultaneidade / sincronização, como discutido aqui . Estes são problemas particularmente difíceis, por várias razões. Em primeiro lugar, como você descobriu, alterações aparentemente não relacionadas podem desencadear o problema (outros erros também podem ter esse efeito, mas os erros de simultaneidade quase sempre acontecem.) Segundo, eles são muito difíceis de diagnosticar: o erro geralmente se manifesta em um local que é distante no tempo ou código da causa, e qualquer coisa que você faça para diagnosticá-lo pode fazer com que ele desapareça ( Heisenbugs) Em terceiro lugar, é muito difícil encontrar erros de concorrência nos testes. Em parte, isso se deve à explosão combinatória: é ruim o suficiente para o código seqüencial, mas adicionar as possíveis intercalações da execução simultânea explode até o ponto em que o problema seqüencial se torna insignificante em comparação. Além disso, mesmo um bom caso de teste só pode desencadear o problema ocasionalmente - Nancy Leveson calculou que um dos erros letais no Therac 25ocorreu em uma das cerca de 350 execuções, mas se você não sabe qual é o erro ou mesmo o que existe, não sabe quantas repetições fazem um teste eficaz. Além disso, apenas o teste automatizado é viável nessa escala e é possível que o driver de teste imponha sutis restrições de tempo, de modo que ele nunca realmente ative o bug (Heisenbugs novamente).

Existem algumas ferramentas para teste de simultaneidade em alguns ambientes, como o Helgrind para código usando pthreads POSIX, mas não sabemos as especificidades aqui. Os testes devem ser complementados com análise estática (ou é o contrário?), Se houver ferramentas adequadas para o seu ambiente.

Para aumentar a dificuldade, os compiladores (e até os processadores, em tempo de execução) geralmente são livres para reorganizar o código de maneiras que às vezes tornam o raciocínio sobre sua segurança de thread muito contra-intuitivo (talvez o caso mais conhecido seja a verificação dupla idioma de bloqueio , embora alguns ambientes (Java, C ++ ...) tenham sido modificados para melhorá-lo.)

Esse código pode ter um problema simples que está causando todos os sintomas, mas é mais provável que você tenha um problema sistêmico que poderia interromper seus planos de adicionar novos recursos. Espero ter convencido você de que você pode ter um sério problema em suas mãos, possivelmente até uma ameaça existencial ao seu produto, e a primeira coisa a fazer é descobrir o que está acontecendo. Se isso revelar problemas de simultaneidade, recomendo fortemente que você os corrija primeiro, antes mesmo de fazer a pergunta se deve refatorar mais em geral e antes de tentar adicionar mais recursos.

Sdenham
fonte
1
Onde você vê "condição de corrida" nos comentários? Onde isso está implícito? Você está fazendo muitas suposições técnicas aqui, mesmo sem o benefício de ver o código.
9788 Robert Harvey
2
@RobertHarvey "Tempo limite definido para dar ao Módulo A algum tempo para fazer as coisas." - praticamente a definição de uma condição de corrida, e os intervalos não são a primeira maneira de lidar com eles. Eu não sou o único a tirar essa inferência. Se não houver um problema aqui, tudo bem, mas o interlocutor precisa saber, já que esses comentários são sinais de alerta para uma sincronização mal tratada.
sdenham
3

Estou lidando com uma grande base de código e recebi alguns meses para refatorar o código existente. O processo de refatoração é necessário, pois em breve precisaremos adicionar muitos novos recursos ao nosso produto [...]

Durante a refatoração, de tempos em tempos eu encontro a classe, o método ou as linhas de código que têm comentários como ["não toque nisso!"]

Sim, você deve refatorar essas peças especialmente . Esses avisos foram colocados pelos autores anteriores para significar "não mexa ociosamente nessas coisas, é muito complicado e há muitas interações inesperadas". Como sua empresa planeja desenvolver ainda mais o software e adicionar muitos recursos, eles especificamente encarregaram você de limpar essas coisas. Então você não está de braços cruzados adulteração com ele, você está deliberadamente carregado com a tarefa de limpeza.

Descubra o que esses módulos complicados estão fazendo e divida-os em problemas menores (o que os autores originais deveriam ter feito). Para obter um código sustentável de uma bagunça, as partes boas precisam ser refatoradas e as partes ruins precisam ser reescritas .

DepressedDaniel
fonte
2

Essa questão é outra variação no debate de quando / como refatorar e / ou limpar o código com uma pitada de "como herdar código". Todos temos experiências diferentes e trabalhamos em organizações diferentes, com equipes e culturas diferentes. Portanto, não há resposta certa ou errada, exceto "faça o que você acha que precisa fazer e faça de uma maneira que não seja demitido". .

Acho que não há muitas organizações que gostariam de ter um processo de negócios de lado, porque o aplicativo de suporte precisava de limpeza ou refatoração de código.

Nesse caso específico, os comentários do código estão aumentando a sinalização de que a alteração dessas seções do código não deve ser feita. Portanto, se você prosseguir e os negócios caírem de lado, além de não ter nada a mostrar para apoiar suas ações, também há um artefato que funciona contra sua decisão.

Portanto, como sempre é o caso, você deve proceder com cuidado e fazer alterações somente depois de entender todos os aspectos do que está prestes a mudar e encontrar maneiras de testar o problema, prestando muita atenção à capacidade, desempenho e tempo devido a os comentários no código.

Mesmo assim, sua gerência precisa entender o risco inerente ao que você está fazendo e concordar que o que você está fazendo tem um valor comercial que supera o risco e que você fez o que pode ser feito para atenuar esse risco.

Agora, voltemos todos aos nossos próprios TODO e as coisas que sabemos podem ser aprimoradas em nossas próprias criações de código, se houver mais tempo.

Thomas Carlisle
fonte
1

Sim absolutamente. Essas são indicações claras de que a pessoa que escreveu esse código estava insatisfeita com o código e provavelmente cutucou-o até conseguir que funcionasse. É possível que eles não entendam quais eram os problemas reais ou, pior, os entendiam e estavam com preguiça de resolvê-los.

No entanto, é um aviso de que será necessário muito esforço para corrigi-los e que essas correções terão riscos associados a elas.

Idealmente, você poderá descobrir qual era o problema e corrigi-lo corretamente. Por exemplo:

Tempo limite definido para dar ao Módulo A algum tempo para fazer as coisas. Se não for cronometrado como este, ele será interrompido.

Isso sugere fortemente que o Módulo A não indica adequadamente quando está pronto para ser usado ou quando terminou o processamento. Talvez a pessoa que escreveu isso não quis se preocupar em consertar o Módulo A ou não possa, por algum motivo. Parece um desastre esperando para acontecer, porque sugere uma dependência de tempo sendo tratada pela sorte, em vez de um seqüenciamento adequado. Se eu visse isso, gostaria muito de corrigi-lo.

Não mude isso. Confie em mim, você vai quebrar as coisas.

Isso não diz muito. Isso dependeria do que o código estava fazendo. Isso pode significar que ele tem otimizações que parecem óbvias que, por um motivo ou outro, realmente quebram o código. Por exemplo, um loop pode deixar uma variável com um valor específico do qual depende outro trecho de código. Ou uma variável pode ser testada em outro encadeamento e a alteração da ordem das atualizações de variáveis ​​pode quebrar esse outro código.

Sei que usar setTimeout não é uma boa prática, mas nesse caso eu tive que usá-lo.

Parece fácil. Você deve ver o que setTimeoutestá fazendo e talvez encontrar uma maneira melhor de fazê-lo.

Dito isto, se esses tipos de correções estiverem fora do escopo do seu refator, isso indica que tentar refatorar dentro deste código pode aumentar significativamente o escopo do seu esforço.

No mínimo, observe atentamente o código afetado e veja se você pode pelo menos melhorar o comentário a ponto de explicar mais claramente qual é o problema. Isso pode salvar a próxima pessoa de enfrentar o mesmo mistério que você enfrenta.

David Schwartz
fonte
2
É possível que as condições tenham mudado para o ponto em que os problemas antigos não existam mais, e os comentários de aviso tenham se tornado o lugar nos mapas antigos que dizem "Aqui estão os dragões". Mergulhe e aprenda qual era a situação ou veja que ela passou para a história.
2
No mundo real, alguns dispositivos não fornecem uma indicação confiável de prontidão, mas especificam um tempo máximo de indisponibilidade. Indicações de prontidão confiáveis ​​e eficientes são boas, mas às vezes a pessoa fica presa usando coisas sem elas.
Supercat
1
@ supercat Talvez, talvez não. Não podemos dizer pelo comentário aqui. Portanto, vale a pena investigar, se nada mais, melhorar o comentário com detalhes.
David Schwartz
1
@DavidSchwartz: O comentário certamente poderia ser mais útil, mas às vezes não está claro quanto tempo um programador deve gastar tentando mapear todas as maneiras precisas em que um dispositivo falha em cumprir sua especificação, especialmente se não está claro se o problema é esperado. ser uma coisa temporária ou permanente.
Supercat
1

O autor dos comentários provavelmente não entendeu completamente o código . Se eles realmente soubessem o que estavam fazendo, teriam escrito comentários realmente úteis (ou não introduziram as condições das corridas em primeiro lugar). Um comentário como " Confie em mim, você quebrará as coisas " . Indica que o autor tenta mudar algo que causou erros inesperados que eles não entenderam completamente.

O código é provavelmente desenvolvido através de adivinhação e tentativa e erro, sem uma compreensão completa do que realmente acontece.

Isso significa:

  1. É arriscado alterar o código. Levará tempo para entender, obviamente, e provavelmente não segue bons princípios de design e pode ter efeitos e dependências obscuros. Provavelmente, ele é insuficientemente testado e, se ninguém entender completamente o que o código faz, será difícil escrever testes para garantir que nenhum erro ou alteração de comportamento seja introduzido. As condições de corrida (como os comentários mencionam) são especialmente onerosas - este é um lugar onde os testes de unidade não o salvam.

  2. É arriscado não mudar o código. O código tem uma alta probabilidade de conter bugs obscuros e condições de corrida. Se um bug crítico é descoberto no código, ou um de alta prioridade forças de mudança requisito de negócios que você altere este código em curto prazo, você está em profunda angústia. Agora você tem todos os problemas descritos em 1, mas sob pressão do tempo! Além disso, essas "áreas escuras" no código tendem a se espalhar e infectar outras partes do código que ele toca.

Uma complicação adicional: os testes de unidade não o salvarão . Geralmente, a abordagem recomendada para esse código legado de correção é adicionar testes de unidade ou integração primeiro e depois isolar e refatorar. Mas as condições de corrida não podem ser capturadas por testes automatizados. A única solução é sentar e refletir sobre o código até que você o entenda e depois reescrever para evitar as condições de corrida.

Isso significa que a tarefa é muito mais exigente do que apenas a refatoração de rotina. Você precisará agendá-lo como uma tarefa de desenvolvimento real.

Você pode encapsular o código afetado como parte da refatoração regular, portanto, pelo menos, o código perigoso é isolado.

JacquesB
fonte
-1

A pergunta que eu faria é por que alguém escreveu: NÃO EDITE em primeiro lugar.

Eu trabalhei em muitos códigos e alguns deles são feios e levaram muito tempo e esforço para começar a trabalhar, dentro das restrições dadas no momento.

Aposto que, neste caso, isso aconteceu e a pessoa que escreveu o comentário descobriu que alguém o alterou e, em seguida, teve que repetir todo o exercício e colocá-lo de volta no caminho, para que a coisa funcionasse. Depois disso, por frustração por cisalhamento, eles escreveram ... NÃO EDITAR.

Em outras palavras, não quero consertar isso novamente, pois tenho coisas melhores a fazer com a minha vida.

Ao dizer NÃO EDITAR, é uma maneira de dizer que sabemos tudo o que jamais saberemos agora e, portanto, no futuro nunca aprenderemos nada de novo.

Deve haver pelo menos um comentário sobre os motivos da não edição. Como dizer "Não toque" ou "Não digite". Por que não tocar a cerca? Por que não entrar? Não seria melhor dizer: "Cerca elétrica, não toque!" ou "Minas terrestres! Não entre!". Então fica óbvio o porquê, e ainda assim você pode entrar, mas pelo menos sabe as consequências antes de entrar.

Aposto também que o sistema não tem testes em torno desse código mágico e, portanto, ninguém pode confirmar que o código funcionará corretamente após qualquer alteração. A colocação de testes de caracterização em torno do código do problema é sempre o primeiro passo. Consulte "Trabalhando com código legado", de Michael Feathers, para obter dicas sobre como quebrar dependências e colocar o código em teste, antes de tentar alterar o código.

Penso que, no final, é míope colocar restrições à refatoração e deixar o produto evoluir de maneira natural e orgânica.

BigA
fonte
2
este não parece oferecer nada substancial sobre os pontos feitos e explicado em antes 9 respostas
mosquito