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

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)?

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".

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.

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.

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".

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.

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.

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.

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.

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 .

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.

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.

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.

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.