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

Parece que você está refatorando "apenas no caso", 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 frágeis, ou se você pode deixá-los como estão.

Para dizer isso: acho que isso é uma estratégia de refatoração condenada . Você está investindo tempo e dinheiro da sua empresa para algo em que ninguém sabe se ela realmente retornará um benefício, e você está no limite de tornar as coisas piores 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 unitários, mas testes de integração) aos módulos em risco. Especialmente aqueles módulos frágeis que você mencionou precisarão de um conjunto completo de testes antes de você alterar qualquer coisa lá.

• refatorie apenas os bits necessários para colocar os testes no lugar. Tente minimizar qualquer uma das mudanças necessárias. A única exceção é quando seus testes revelam bugs - então corrija-os imediatamente (e refatore-os na medida em que você precise fazê-lo com segurança).

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

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

Assim, quando chegar a hora de conhecer com certeza , será necessário alterar e refatorar os módulos frágeis (devido ao novo desenvolvimento do recurso ou porque os testes adicionados na etapa 1 revelam alguns bugs ), então você e sua equipe estão prontos para refatorar os módulos e, com maior ou menor segurança, ignorar esses comentários de aviso.

Como resposta a alguns comentários : para ser justo, se suspeitarmos que um módulo em um produto existente é a causa dos problemas regularmente, especialmente um módulo marcado como "don toque ", concordo com todos vocês. Ele deve ser revisado, depurado e, provavelmente, refatorado nesse processo (com o suporte dos testes que mencionei, e não necessariamente nessa ordem). Bugs são uma strong justificativa para a mudança, muitas vezes mais strong do que os novos recursos. No entanto, esta é uma decisão caso a caso. É preciso verificar com muito cuidado se realmente vale a pena mudar 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 particulares do ambiente no qual 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 no SO, alterações não relacionadas em outro componente do sistema, alterações em volumes de dados típicos, o nome dele. Não há garantia de que nos dias de hoje 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 que você talvez nunca perceba). É por isso que programar um atraso fixo para permitir que outro componente termine está quase sempre incorreto e funciona apenas acidentalmente.

No seu caso, você não conhece as circunstâncias originais e nem pode perguntar aos autores originais. (Presumivelmente, você também não tem testes de regressão / integração adequados, ou pode simplesmente ir em frente e deixar que seus testes lhe digam 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 nesse ponto já está lá. É muito melhor perturbar o carrinho de maçã agora , quando a única coisa que você está fazendo é a refatoração, e ter certeza de que se as coisas quebrarem foi a refatoração que causou isso, do que esperar até você estão fazendo alterações adicionais simultaneamente e nunca se esqueça.

My question is: should I refactor the code when I encounter such warnings from the authors

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

for now we are no longer able to add any feature without breaking something else

Agora você precisa se concentrar em aumentar a estabilidade e não em refatorar. Você pode fazer a refatoração 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 legado, então você precisa tratá-la de forma um pouco diferente.

Comece adicionando testes de caracterização. Não se preocupe com nenhuma especificação, basta adicionar um teste que confirme o comportamento atual. Isso ajudará a impedir que novos trabalhos interrompam 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ê adicionar um novo recurso, adicione pelo menos alguns testes básicos para que o novo recurso funcione como desejado.

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

I was given a few months to refactor existing code.

Comece obtendo cobertura de teste. Comece com as áreas que mais quebram. Comece com as áreas que mais mudam. Então, depois de identificar os designs ruins, substitua-os um por um.

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

Lembre-se de a cerca do GK Chesterton : não derrube uma cerca obstruindo uma estrada até entender por que foi construído.

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 examinar 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 ele foi marcado com um aviso na época e o que aconteceria se o aviso fosse ignorado.

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

Código com comentários como os que você mostrou seriam os principais itens da minha lista para refatorar se, e somente se eu tiver alguma razão para isso . A questão é que o código é tão ruim que você cheira nos comentários. É inútil tentar usar qualquer nova funcionalidade nesse código, esse código precisa morrer assim que precisar mudar.

Observe também que os comentários não são úteis no mínimo: eles só dão aviso e não há razão. Sim, eles são os sinais de alerta em volta de um tanque de tubarões. Mas se você quiser fazer alguma coisa na vizinhança, há poucos pontos para tentar nadar com os tubarões. Você deve se livrar desses tubarões primeiro.

Dito isto, você absolutamente precisa de bons casos de teste antes de se atrever a trabalhar em tal código. Depois de ter esses casos de teste, certifique-se de entender cada pequena parte que você está mudando, para garantir que você realmente não está mudando o comportamento. Faça da sua maior prioridade manter todas as peculiaridades comportamentais do código até que você possa provar que elas estão sem nenhum efeito. Lembre-se, você está lidando com tubarões - você deve ter muito cuidado.

Assim, no caso do tempo limite: deixe-o entrar 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 você proceder para removê-lo.

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

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

Provavelmente não é uma boa ideia refatorar código com tais avisos, se as coisas funcionarem bem como estão.

Mas se você precisar refatorar ...

Primeiro, escreva alguns testes de unidade e testes de integração para testar as condições que os avisos estão lhe alertando. Tente imitar tanto quanto possível as condições de produção . Isso significa espelhar o banco de dados de produção para um servidor de teste, executar os mesmos 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 em seu novo código para ver se você pode obter os mesmos resultados que o original. Se você puder, então é provavelmente OK para implementar sua refatoração na produção. Mas esteja pronto para reverter as mudanças se as coisas derem errado.

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

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

A questão central aqui é que (como observado em algumas respostas) os comentários citados indicam strongmente 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, mudanças aparentemente não relacionadas podem desencadear o problema (outros erros também podem ter esse efeito, mas quase sempre ocorrem erros de simultaneidade). Segundo, eles são muito difíceis de diagnosticar: o bug geralmente se manifesta em um lugar que é distante no tempo ou no código da causa, e qualquer coisa que você fizer para diagnosticar isso pode fazer com que ela desapareça ( Heisenbugs ). Em terceiro lugar, os erros de concorrência são muito difíceis de encontrar nos testes. Em parte, isso se deve à explosão combinatória: ela é ruim o suficiente para o código sequencial, mas a adição de possíveis intercalações da execução simultânea leva ao 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 bugs letais no Therac 25 ocorreu em uma das cerca de 350 execuções, mas se você não sabe o que é o bug, ou mesmo que existe, você não sabe quantas repetições fazem um teste efetivo. Além disso, somente o teste automatizado é viável nessa escala, e é possível que o driver de teste imponha restrições sutis de tempo, de modo que ele nunca realmente acione o bug (Heisenbugs novamente).

Existem algumas ferramentas para testes de simultaneidade em alguns ambientes, como Helgrind para código usando POSIX pthreads , mas não sabemos os detalhes aqui. O teste deve ser complementado 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é mesmo os processadores, em tempo de execução) são geralmente livres para reorganizar o código de maneiras que às vezes tornam o raciocínio sobre sua segurança de thread bastante contra-intuitivo (talvez o caso mais conhecido seja o bloqueio duplo , 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 possa interromper seus planos de adicionar novos recursos. Espero tê-lo convencido 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 enfaticamente que você os corrija primeiro, antes mesmo de fazer a pergunta se você deve fazer uma refatoração mais geral e antes de tentar adicionar mais recursos.

I am dealing with a pretty big codebase and I was given a few months to refactor existing code. The refactor process is needed because soon we will need to add many new features to our product [...]

During refactoring, from time to time I encounter the class, method or lines of code which have comments like ["don't touch this!"]

Sim, você deve refatorar essas partes especialmente . Esses avisos foram colocados lá pelos autores anteriores para significar "não oculte adulterar essas coisas, é muito complicado e há muitas interações inesperadas". Como sua empresa planeja desenvolver ainda mais o software e adicionar muitos recursos, eles especificamente o encarregaram de limpar essas coisas. Então você não está ociosamente mexendo com isso, você está deliberadamente encarregado da tarefa de limpá-lo.

Descubra o que esses módulos complicados estão fazendo e decompô-lo em problemas menores (o que os autores originais deveriam ter feito). Para obter um código de fácil manutenção, as partes boas precisam ser refatoradas e as partes ruins precisam ser reescritas .

Esta questão é outra variação do debate sobre quando / como refatorar e / ou limpar o código com um traço de "como herdar código". Todos nós temos experiências diferentes e trabalhamos em diferentes organizações com diferentes equipes e culturas, então não há nenhuma resposta certa ou errada, exceto "faça o que você acha que precisa fazer, e faça isso de uma maneira que não o faça ser demitido". .

Neste caso específico, os comentários de código estão levantando a atenção de que a alteração dessas seções de código não deve ser feita. Então, se você continuar, e o negócio cair em seu lado, não apenas você não tem nada para mostrar para apoiar suas ações, mas sim um artefato que funciona contra sua decisão.

Então, como sempre acontece, você deve proceder com cuidado e fazer mudanças somente depois de entender cada aspecto do que está prestes a mudar, e encontrar maneiras de testar o problema, prestando muita atenção à capacidade e desempenho e temporização por causa dos comentários no código.

Mas, 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 mitigar esse risco.

Agora, vamos todos voltar para o nosso próprio TODO e as coisas que sabemos podem ser melhoradas em nossas próprias criações de código se houvesse mais tempo.

Sim, absolutamente. Essas são indicações claras de que a pessoa que escreveu esse código não estava satisfeita com o código e provavelmente o cutucou até que ele funcionasse. É possível que eles não entendessem quais eram os problemas reais ou, pior, entendessem e estivessem com preguiça de consertá-los.

No entanto, é um aviso de que serão necessários muitos esforços para corrigi-los e que tais correções terão riscos associados a eles.

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

Time out set to give Module A some time to do stuff. If its not timed like this, it will break.

Isso sugere strongmente que o Módulo A não indica corretamente quando está pronto para ser usado ou quando termina o processamento. Talvez a pessoa que escreveu isso não quisesse se preocupar em consertar o Módulo A ou não pudesse por algum motivo. Isso parece um desastre esperando para acontecer porque sugere uma dependência de tempo sendo tratada pela sorte e não pelo sequenciamento adequado. Se eu visse isso, gostaria muito de corrigir isso.

Do not change this. Trust me, you will break things.

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

I know using setTimeout is not a good practice, but in this case I had to use it.

Isso parece fácil. Você deve ser capaz de ver o que o setTimeout está fazendo e talvez encontre uma maneira melhor de fazê-lo.

Dito isso, se esses tipos de correções estiverem fora do escopo de seu refatorador, essas são indicações de que tentar refatorar dentro desse código pode aumentar significativamente o escopo de 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 com mais clareza 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 em si . Eles realmente sabiam o que estavam fazendo, eles teriam escrito comentários realmente úteis (ou não introduziram as condições de corrida em primeiro lugar). Um comentário como " Confie em mim, você vai quebrar as coisas. " para mim indica que o autor tenta alterar algo que causou erros inesperados que eles não entenderam completamente.

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

Isso significa:

1. É arriscado mudar 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. É mais provável que seja insuficientemente testado, e se ninguém entender completamente o que o código faz, então será difícil escrever testes para garantir que nenhum erro ou mudança no comportamento seja introduzido. As condições de corrida (como os comentários fazem alusão a) são especialmente onerosas - este é um lugar onde os testes de unidade não vão te salvar.

2. É arriscado não alterar 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 uma mudança de requisito de negócios de alta prioridade obriga você a alterar este código em pouco tempo, você está com problemas deep . Agora você tem todos os problemas descritos em 1, mas sob pressão de tempo! Além disso, tais "áreas escuras" no código têm uma tendência a se espalhar e infectar outras partes do código que toca.

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

Isso significa que a tarefa é muito mais exigente do que apenas a refatoração de rotina. Você terá que agendar isso como uma tarefa de desenvolvimento real.

Você pode encapsular o código afetado como parte da refatoração regular, de modo que pelo menos o código perigoso seja isolado.

A pergunta que faço é por que alguém escreveu, NÃO EDITAR em primeiro lugar.

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

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

Em outras palavras, não quero ter que 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 vamos conhecer agora e, portanto, no futuro, nunca aprenderemos nada de novo.

Deve haver pelo menos um comentário sobre os motivos para não editar. Como dizer "Não toque" ou "Não entre". Por que não tocar na 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 é óbvio por que, e ainda assim você ainda pode entrar, mas pelo menos saiba as conseqüências antes de fazer isso.

Eu também aposto que o sistema não tem testes em torno deste código mágico e, portanto, ninguém pode confirmar que o código funcionará corretamente após qualquer alteração. Colocar testes de caracterização em torno do código do problema é sempre um primeiro passo. Veja "Trabalhando com Código Legado", de Michael Feathers, para obter dicas sobre como quebrar dependências e obter o código em teste, antes de tentar alterar o código.

Eu acho que, no final, ele está em falta para colocar restrições à refatoração e permitir que o produto evolua de maneira natural e orgânica.