Reconciliando a regra dos escoteiros e a refatoração oportunista com revisões de código

55

Acredito muito na regra dos escoteiros :

Sempre verifique um módulo mais limpo do que quando você fez o check-out. "Não importa quem seja o autor original, e se sempre fizermos algum esforço, por menor que seja, para melhorar o módulo. Qual seria o resultado? Acho que se todos seguissem essa regra simples, veríamos o fim da implacável deterioração de nossos sistemas de software.Em vez disso, nossos sistemas gradualmente se tornariam cada vez melhores à medida que evoluíam.Também veríamos equipes cuidando do sistema como um todo. do que apenas indivíduos cuidando de sua própria pequena parte.

Também acredito muito na ideia relacionada de Refatoração Oportunista :

Embora existam lugares para alguns esforços agendados de refatoração, prefiro incentivar a refatoração como uma atividade oportunista, realizada sempre e onde o código precisar ser limpo - por quem quer que seja. O que isso significa é que, a qualquer momento, alguém vê algum código que não é tão claro quanto deveria ser, deve aproveitar a oportunidade para corrigi-lo ali mesmo - ou pelo menos dentro de alguns minutos

Observe particularmente o seguinte trecho do artigo de refatoração:

Desconfio de qualquer prática de desenvolvimento que cause atrito para a refatoração oportunista ... Meu senso é que a maioria das equipes não faz refatoração suficiente, por isso é importante prestar atenção a qualquer coisa que esteja desencorajando as pessoas a fazê-lo. Para ajudar a esclarecer isso, fique atento a qualquer momento em que você se sentir desencorajado de fazer uma pequena refatoração, uma que, com certeza, levará apenas um ou dois minutos. Qualquer barreira desse tipo é um cheiro que deve desencadear uma conversa. Portanto, anote o desânimo e traga à equipe. No mínimo, isso deve ser discutido durante sua próxima retrospectiva.

Onde trabalho, há uma prática de desenvolvimento que causa atritos pesados - Revisão de Código (CR). Sempre que mudo algo que não esteja no escopo da minha "tarefa", sou criticado pelos meus revisores por dificultar a revisão. Isso é especialmente verdadeiro quando a refatoração está envolvida, pois dificulta a comparação de diferenças "linha a linha". Essa abordagem é o padrão aqui, o que significa que a refatoração oportunista raramente é feita, e apenas a refatoração "planejada" (que geralmente é muito pouco, muito tarde) ocorre, se é que existe.

Eu afirmo que os benefícios valem a pena e que três revisores trabalharão um pouco mais (para realmente entender o código antes e depois, em vez de olhar para o escopo restrito de quais linhas foram alteradas - a própria revisão seria melhor apenas por causa disso. ) para que os próximos 100 desenvolvedores que leem e mantenham o código se beneficiem. Quando apresento esse argumento, meus revisores dizem que não têm problemas com minha refatoração, desde que não esteja no mesmo CR. No entanto, afirmo que isso é um mito:

(1) Na maioria das vezes, você só percebe o que e como deseja refatorar quando está no meio de sua tarefa. Como Martin Fowler coloca:

Ao adicionar a funcionalidade, você percebe que algum código que está adicionando contém alguma duplicação com o código existente, portanto, é necessário refatorar o código existente para limpar as coisas ... Você pode conseguir algo funcionando, mas percebe que seria melhor se a interação com as classes existentes tiver sido alterada. Aproveite a oportunidade para fazer isso antes de se considerar terminado.

(2) Ninguém vai olhar favoravelmente para você liberando CRs "refatorando" que você não deveria fazer. Um CR tem uma certa sobrecarga e seu gerente não quer que você "perca seu tempo" em refatoração. Quando está incluído na alteração que você deve fazer, esse problema é minimizado.

O problema é exacerbado pelo Resharper, pois cada novo arquivo que adiciono à alteração (e não posso saber com antecedência exatamente quais arquivos acabariam sendo alterados) geralmente está repleto de erros e sugestões - a maioria dos quais está no local e merece totalmente fixação.

O resultado final é que eu vejo código horrível e deixo lá. Ironicamente, sinto que a fixação desse código não apenas não melhorará minha classificação, mas na verdade os reduzirá e me pintará como o cara "sem foco" que perde tempo consertando coisas que ninguém se importa em vez de fazer seu trabalho. Eu me sinto mal por isso, porque realmente desprezo o código incorreto e não suporto vê-lo, muito menos chamá-lo de meus métodos!

Alguma idéia de como posso remediar esta situação?

t0x1n
fonte
40
Eu me sentiria desconfortável de trabalho em um lugar ondeyour manager doesn't want you to "waste your time" on refactoring
Daenyth
19
Além de ter vários CRs, o ponto principal é que cada confirmação deve ter um único objetivo: um para o refator, um para o requisito / bug / etc. Dessa forma, uma revisão pode diferenciar entre o refator e a alteração do código solicitado. Eu também argumentaria que o refatorador só deve ser feito se houver testes de unidade em andamento que comprovem que o refator não quebrou nada (o tio Bob concorda).
2
@ t0x1n Eu não vejo isso como qualquer diferente
Daenyth
2
@ t0x1n sim, eu perdi essa. Não há café suficiente esta manhã. Na minha experiência, existem algumas maneiras de refatorar. Talvez você observe o código que precisa modificar e saiba imediatamente que ele precisa de limpeza, e faça isso primeiro. Talvez você precise refatorar algo para fazer sua alteração, porque o novo requisito é incompatível com o código existente. Eu argumentaria que o refator é intrinsecamente parte de sua alteração e não deve ser considerado separado. Por fim, talvez você veja que o código é péssimo no meio da alteração, mas você pode finalizá-lo. Refatorar após o fato.
6
O marcador 1 não afirma que é impossível separar os commits. Isso implica apenas que você não sabe como fazê-lo, ou o seu VCS dificulta. Eu faço isso o tempo todo, mesmo tendo um único commit e dividindo-o após o fato.
Useless

Respostas:

18

OK, então agora há mais coisas do que são adequadas para um comentário.

tl; dr

Sua intuição sobre o que você deve fazer (refatorar à medida que avança) está correta.

Sua dificuldade em implementar isso - considerando que você precisa solucionar um sistema de revisão de código ruim - se resume a sua dificuldade em manipular seu código fonte e o VCS. Várias pessoas disseram que você pode e deve dividir suas alterações (sim, mesmo em arquivos) em várias confirmações, mas você parece ter dificuldade em acreditar nisso.

Você realmente pode fazer isso. Isso é realmente o que estamos sugerindo. Você realmente deve aprender a tirar o máximo proveito de suas ferramentas de edição, manipulação de fontes e controle de versão. Se você investe tempo em aprender a usá-los bem, isso facilita a vida.


Problemas de política de fluxo de trabalho / escritório

Eu faria essencialmente a mesma recomendação que o GlenH7 de criar dois commits - um com apenas refatorações e (demonstrável ou obviamente) sem alterações funcionais e outro com as alterações funcionais em questão.

Pode ser útil, se você encontrar muitos erros, escolher uma única categoria de erro para corrigir em um único CR. Então você tem um commit com um comentário como "código de deduplicação", "corrija erros do tipo X" ou qualquer outra coisa. Como isso faz um único tipo de alteração, presumivelmente em vários lugares, deve ser trivial revisar. Isso significa que você não pode corrigir todos os erros encontrados, mas pode tornar menos doloroso o contrabando.


Problemas VCS

Dividir as alterações feitas na sua fonte de trabalho em várias confirmações não deve ser um desafio. Você não disse o que está usando, mas os possíveis fluxos de trabalho são:

  1. se você estiver usando git, você tem excelentes ferramentas para isso

    • você pode usar git add -ipara teste interativo na linha de comando
    • você pode usar git guie selecionar blocos e linhas individuais para montar (esta é uma das poucas ferramentas GUI relacionadas ao VCS que eu realmente prefiro à linha de comando, a outra sendo um bom editor de mesclagem de 3 vias)
    • você pode fazer muitos commits minúsculos (alterações individuais ou corrigir a mesma classe de bug em vários locais) e depois reordená-los, mesclá-los ou dividi-los com rebase -i
  2. se você não estiver usando o git, seu VCS ainda pode ter ferramentas para esse tipo de coisa, mas não posso aconselhar sem saber qual sistema você usa

    Você mencionou que está usando o TFS - que acredito ser compatível com o git desde o TFS2013. Pode valer a pena experimentar o uso de um clone git local do repositório para trabalhar. Se isso estiver desativado ou não funcionar, você ainda poderá importar a fonte para um repositório git local, trabalhar nele e usá-lo para exporte seus commits finais.

  3. você pode fazer isso manualmente em qualquer VCS se tiver acesso a ferramentas básicas como diffe patch. É mais doloroso, mas certamente possível. O fluxo de trabalho básico seria:

    1. faça todas as suas alterações, teste etc.
    2. use diffpara criar um arquivo de patch (contexto ou unificado) com todas as alterações desde a última confirmação
    3. particione isso em dois arquivos: você terminará com um arquivo de correção contendo refatorações e outro com alterações funcionais
      • isso não é inteiramente trivial - ferramentas como o modo diff do emacs podem ajudar
    4. faça backup de tudo
    5. reverter para a última confirmação, use patchpara reproduzir um dos arquivos de correção, confirmar essas alterações
      • NB se o patch não foi aplicado corretamente, pode ser necessário corrigir os pedaços com falha manualmente
    6. repita 5 para o segundo arquivo de patch

Agora você tem dois commits, com suas alterações particionadas adequadamente.

Note que provavelmente existem ferramentas para facilitar esse gerenciamento de patches - simplesmente não as uso, pois o git faz isso por mim.

Sem utilidade
fonte
Não tenho certeza de que sigo - o bullet 3.3 pressupõe que as alterações de refatoração e funcionais residem em arquivos diferentes? De qualquer forma, eles não. Talvez separar por linhas faça mais sentido, mas acho que não temos ferramentas para isso em nosso CVS (TFS). De qualquer forma, não funcionaria para muitas refatorações (a maioria?) Nas quais as alterações funcionais dependem das alterações refatoradas. Por exemplo, suponha que refatore o método Foo (que eu preciso usar como parte da minha alteração funcional) para obter 3 parâmetros em vez de 2. Agora, o código funcional da Imy depende do código de refatoração, mesmo a divisão por linha não ajuda.
t0x1n 12/06
11
Linhas diferentes no mesmo arquivo são adequadas nos fluxos de trabalho fornecidos. E, como os dois commits seriam seqüenciais , é perfeitamente aceitável que o segundo commit (funcional) dependa do primeiro. Ah, e o TFS2013 supostamente suporta o git.
Inútil
Também usamos o TFS para o controle de origem. Você assume que o segundo commit será o funcional, enquanto que normalmente seria o contrário (visto que não posso dizer de antemão que refatoração teria que ser feita). Suponho que eu poderia fazer todo o meu trabalho de refatoração funcional +, depois me livrar de qualquer coisa funcional e adicioná-lo novamente em um commit separado. Só estou dizendo, isso é um monte de problemas (e tempo) apenas para manter alguns revisores felizes. Uma abordagem mais razoável para minha mente é permitir a refatoração oportunista e, em troca, concordar com a RC, tais mudanças (aceitando a dificuldade adicional).
t0x1n 12/06
3
Eu acho que você realmente não está me entendendo. Editar edições de origem e de agrupamento em confirmações, sim, mesmo edições no mesmo arquivo, são atividades logicamente separadas. Se isso parecer difícil, você só precisa aprender melhor as ferramentas de gerenciamento de código-fonte disponíveis.
Inútil
11
Sim, seu entendimento está correto, você teria duas confirmações seqüenciais com a segunda (funcional), dependendo da primeira (refatoração). O fluxo de trabalho diff / patch descrito acima é precisamente uma maneira de fazer isso que não requer exclusão manual de alterações e, em seguida, reescrevê-las.
inútil
29

Vou assumir que as Solicitações de Mudança são grandes e formais na sua empresa. Caso contrário, faça as alterações (sempre que possível) em muitos commits pequenos (como você deveria).

Alguma idéia de como posso remediar esta situação?

Continuar fazendo o que está fazendo?

Quero dizer, todos os seus pensamentos e deduções estão totalmente corretos. Você deve consertar as coisas que vê. As pessoas não fazem a refatoração planejada o suficiente. E esse benefício para todo o grupo é mais importante do que incomodar alguns.

O que pode ajudar é ser menos combativo. As revisões de código não devem ser combativas "Por que você fez isso?", Elas devem ser colaborativas "Ei, pessoal, enquanto eu estava aqui, eu arrumei tudo isso!". Trabalhar (com seu líder / gerente, se possível) para mudar essa cultura é difícil, mas é vital para criar uma equipe de desenvolvimento com alto desempenho.

Você também pode trabalhar (com seu líder / gerente, se possível) para promover a importância dessas idéias com seus colegas. Faça isso perguntando "por que vocês não se importam com a qualidade?" em vez de perguntar "por que você está sempre fazendo essas coisas inúteis?".

Telastyn
fonte
5
Sim, os CRs são grandes e formais. As alterações são CRed, desconectadas e enviadas para uma fila. Pequenas alterações devem ser adicionadas como iterações a um CR em andamento, em vez de confirmadas separadamente. O WRT continuando o que estou fazendo, que pode realmente beneficiar o grupo, mas temo que não me beneficie . As pessoas que "incomodo" são provavelmente as mesmas que me classificariam na revisão anual. O problema de mudar a cultura é que os grandes chefes acreditam nela. Talvez eu só preciso ganhar mais respeito em seus olhos antes de tentar algo assim ...
t0x1n
13
@ t0x1n - Não olhe para mim. Eu fiz uma carreira de fazer a coisa certa diante de pessoas que são teimosamente apegadas à sucção. Talvez não seja tão lucrativo quanto eu poderia ter feito as pessoas felizes, mas durmo bem.
Telastyn
Obrigado por ser honesto. É de fato algo a se considerar.
t0x1n 12/06
11
Eu também frequentemente encontro isso. Garantir que eu tenho um patch de "limpeza" e um patch de trabalho ajuda muito. Geralmente luto dentro do sistema e depois saio para trabalhar em algum lugar menos estressante. Dito isto, às vezes há razões válidas para as preocupações de seus colegas de trabalho. Por exemplo, se o código entrar rapidamente em produção e não houver testes suficientes. Eu vi a revisão de código como uma tentativa de evitar testes. Não funciona. A revisão de código ajuda a manter um corpo de código uniforme. Faz pouco para erros.
Sean Perry
11
@SeanPerry aggreed - mas eu estou falando sobre circunstâncias normais, onde existem testes, festanças de bugs serão realizados, etc.
t0x1n
14

Sinto muita simpatia por sua situação, mas poucas sugestões concretas. Se nada mais, talvez eu o convença que, por pior que seja a situação, poderia ser pior. SEMPRE pode ser pior. :-)

Primeiro, acho que você tem (pelo menos) dois problemas com sua cultura, não apenas um. Um problema é a abordagem da refatoração, mas as revisões de código parecem um problema distinto. Vou tentar separar meus pensamentos.

Revisões de código

Eu estava em um grupo que odiava revisões de código. O grupo foi formado pela fusão de dois grupos de partes separadas da empresa. Eu vim do grupo que fazia revisões de código há vários anos, com resultados geralmente bons. A maioria de nós acreditava que as revisões de código eram um bom uso de nosso tempo. Nós nos fundimos em um grupo maior, e o mais próximo que pudemos dizer, que "outro" grupo nunca tinha ouvido falar em revisões de código. Agora estávamos trabalhando na base de código "deles".

As coisas não estavam bem quando nos fundimos. Os novos recursos demoravam 6 a 12 meses, ano após ano. A lista de pendências de bugs era enorme, crescente e drenava a vida. A propriedade do código era forte, especialmente entre os "gurus" mais antigos. "Ramos de recursos" às vezes duravam anos e se estendiam a alguns lançamentos; às vezes NINGUÉM, mas um único desenvolvedor viu o código antes de atingir o ramo principal. Na verdade, o "ramo de recursos" é enganoso, pois sugere que o código estava em algum lugar do repositório. Mais frequentemente, era apenas no sistema individual do desenvolvedor.

A gerência concordou que precisávamos "fazer alguma coisa" antes que a qualidade se tornasse inaceitavelmente baixa. :-) A resposta deles foi Code Reviews. A Code Reviews tornou-se um item oficial "Tu Shalt", antes de cada check-in. A ferramenta que usamos foi o Painel de Revisão.

Como funcionou na cultura que descrevi? Melhor do que nada, mas foi doloroso e levou mais de um ano para atingir uma espécie de nível mínimo de conformidade. Algumas coisas que observamos:

  1. A ferramenta usada tende a focar as revisões de código de determinadas maneiras. Isto pode ser um problema. O Painel de Revisão fornece diferenças de linha por linha agradáveis ​​e coloridas e permite anexar comentários a uma linha. Isso faz com que a maioria dos desenvolvedores ignore todas as linhas que não mudaram. Isso é bom para pequenas mudanças isoladas. Não é bom para grandes mudanças, grandes trechos de novo código ou código que possui 500 instâncias de uma função renomeada combinadas com 10 linhas de nova funcionalidade.
  2. Embora estivéssemos em uma base de código antiga e doentia que nunca havia sido revisada antes, tornou-se "indelicado" para um revisor comentar sobre algo que NÃO era uma linha de mudança, mesmo para apontar um bug óbvio. "Não me incomode, este é um check-in importante e não tenho tempo para corrigir bugs."
  3. Compre um revisor "fácil". Algumas pessoas analisam uma revisão de 10 arquivos com 2000 linhas alteradas por 3 minutos e clicam em "Enviar!". Todo mundo aprende rapidamente quem são essas pessoas. Se você realmente não deseja que seu código seja revisto em primeiro lugar, envie-o para um revisor "fácil". O check-in não será mais lento. Você pode retribuir o favor, tornando-se um revisor "fácil" de seu código.
  4. Se você odeia revisões de código, ignore os e-mails recebidos do Painel de Revisão. Ignore os acompanhamentos dos membros da sua equipe. Por semanas. Até você ter três dezenas de análises abertas em sua fila e seu nome aparecer em reuniões de grupo algumas vezes. Torne-se um revisor "fácil" e limpe todos os seus comentários antes da hora do almoço.
  5. Evite enviar seu código para um revisor "rígido". (O tipo de desenvolvedor que faria ou responderia a uma pergunta como essa.) Todo mundo aprende rapidamente quem são os revisores "rígidos", assim como os mais fáceis. Se as revisões de código forem um desperdício de tempo (™), a leitura de feedback detalhado sobre o código que você possui é um desperdício de tempo (™) e um insulto.
  6. Quando as revisões de código são dolorosas, as pessoas as adiam e continuam escrevendo mais código. Você recebe menos revisões de código, mas cada uma é GRANDE. Você precisa de mais análises de código menores, o que significa que a equipe precisa descobrir como torná-las o mais simples possível.

Eu pensei que iria escrever alguns parágrafos sobre Revisões de Código, mas acontece que eu estava escrevendo principalmente sobre cultura. Eu acho que tudo se resume a isso: as revisões de código podem ser boas para um projeto, mas uma equipe obtém apenas os benefícios que eles merecem.

Refatoração

Meu grupo desprezou a refatoração ainda mais do que odiava as revisões de código? Claro! Por todas as razões óbvias que já foram mencionadas. Você está desperdiçando meu tempo com uma revisão de código nojenta e nem está adicionando um recurso ou corrigindo um bug!

Mas o código ainda precisava desesperadamente de refatoração. Como proceder?

  1. Nunca misture uma alteração de refatoração com uma alteração funcional. Várias pessoas mencionaram esse ponto. Se as revisões de código forem um ponto de atrito, não aumente. É mais trabalhoso, mas você deve planejar uma revisão separada e um check-in separado.
  2. Comece pequeno. Muito pequeno. Ainda menor que isso. Use refatorações muito pequenas para ensinar gradualmente às pessoas que refatoração e revisão de código não são pura maldade. Se você puder esgueirar-se em uma pequena refatoração por semana sem muita dor, em alguns meses poderá conseguir fazer duas por semana. E eles podem ser um pouco maiores. Melhor que nada.
  3. Não tivemos essencialmente testes unitários. Então a refatoração é proibida, certo? Não necessariamente. Para algumas alterações, o compilador é o seu teste de unidade. Concentre-se nas refatorações que você pode testar. Evite alterações se elas não puderem ser testadas. Talvez gaste o tempo escrevendo testes de unidade.
  4. Alguns desenvolvedores têm medo de refatorar porque têm medo de TODAS as alterações de código. Levei muito tempo para reconhecer esse tipo. Ele escreve um pedaço de código, mexe nele até que "funcione" e NUNCA deseja alterá-lo. Eles não necessariamente entendem POR QUE funciona. Mudança é arriscada. Eles não confiam em si mesmos para fazer QUALQUER mudança, e certamente não confiarão em você. A refatoração é sobre pequenas mudanças seguras que não alteram o comportamento. Existem desenvolvedores para quem a própria idéia de uma "mudança que não altera o comportamento" é inconcebível . Não sei o que sugerir nesses casos. Eu acho que você precisa tentar trabalhar em áreas com as quais eles não se importam. Fiquei surpreso quando soube que esse tipo pode demorar muito,
GraniteRobert
fonte
11
Esta é uma resposta super atenciosa, obrigado! Concordo especialmente com como a ferramenta de RC pode afetar o foco da revisão ... linha por linha é a saída mais fácil, que não envolve realmente entender o que estava acontecendo antes e o que acontece agora. E, claro, o código que não fez a mudança é perfeito, não há necessidade de olhar sempre em que ...
t0x1n
11
" Poderia ser pior poderia ser rainin'.. " Quando eu li o seu último parágrafo (segundo ponto # 4) eu estava pensando: eles precisam de mais revisão, codificador de revisão. E alguma refatoração, como em: "yourSalary = 0"
Absolutamente local em tantas frentes, resposta incrível. Eu posso ver totalmente de onde você vem: estou exatamente na mesma situação e é incrivelmente frustrante. Você está nessa luta constante por qualidade e boas práticas e não há suporte da parte da gerência, mas ESPECIALMENTE de outros desenvolvedores da equipe.
precisa saber é
13

Por que você não faz as duas coisas, mas com confirmações separadas?

Seus colegas têm razão. Uma revisão de código deve avaliar o código que foi alterado por outra pessoa. Você não deve tocar no código que está revendo para outra pessoa, pois isso influencia sua função como revisor.

Mas se você vir vários problemas flagrantes, há duas opções que você pode seguir.

  1. Se a revisão do código não for boa, permita que a parte que você revisou seja confirmada e refatore o código em um segundo check-in.

  2. Se a revisão do código tiver problemas que precisem de correção, solicite ao desenvolvedor que refatorar com base nas recomendações do resharper.


fonte
Pela sua resposta, entendo que você acredita na Propriedade Forte do Código. Convido você a ler os pensamentos de Fowler sobre por que é uma má idéia: martinfowler.com/bliki/CodeOwnership.html . Para abordar seus pontos especificamente: (1) é um mito, pois a refatoração acontece enquanto você está fazendo sua alteração - não antes nem depois de uma maneira separada, limpa e sem relação, como exigiriam CRs separadas. (2) Com a maioria dos desenvolvedores, isso nunca acontecerá. Nunca . A maioria dos desenvolvedores não se preocupa com essas coisas, muito menos quando vem de outro cara que não é o gerente deles. Eles têm suas próprias coisas que querem fazer.
t0x1n 12/06
8
@ t0x1n: Se seus gerentes não se importam com a refatoração e seus colegas desenvolvedores não se importam com a refatoração ... então a empresa está afundando lentamente. Fugir! :)
logc 12/06
8
@ t0x1n só porque você faz as alterações juntos, não significa que você precisa confirmá- las. Além disso, geralmente é útil verificar se a refatoração não teve efeitos colaterais inesperados separadamente da verificação de que a alteração funcional teve o efeito esperado. Obviamente, isso pode não se aplicar a todas as refatorações, mas não é um mau conselho em geral.
inútil
2
@ t0x1n - Não me lembro de dizer nada sobre propriedade forte de código. Minha resposta foi manter seu papel de revisor puro. Se um revisor introduzir alterações, ele não será mais apenas um revisor.
3
@ GlenH7 Talvez tenha havido algum mal-entendido aqui - eu não sou o revisor. Estou apenas codificando o que preciso e, ao executar o código, posso melhorar o processo. Meus revisores reclamam quando o faço.
t0x1n 12/06
7

Pessoalmente, odeio a ideia de revisar o código pós-commit. A revisão do código deve ocorrer à medida que você faz as alterações. É claro que estou falando aqui de programação em pares. Quando você emparelha, recebe a revisão gratuitamente e uma revisão de código melhor. Agora, estou expressando minha opinião aqui, sei que outros compartilham isso, provavelmente existem estudos que comprovam isso.

Se você conseguir que seus revisores de código emparelhem com você, o elemento combativo da revisão de código deve evaporar. Quando você começa a fazer uma mudança que não é compreendida, a questão pode ser levantada no ponto da mudança, discutida e exploradas alternativas que podem levar a melhores refatorações. Você obterá uma revisão de código de maior qualidade, pois o par será capaz de entender o escopo mais amplo da alteração, e não se concentrará tanto nas alterações linha a linha, que é o que você obtém na comparação lado a lado do código.

É claro que essa não é uma resposta direta ao problema de refatorações estarem fora do escopo da mudança que está sendo trabalhada, mas eu esperaria que seus colegas entendessem melhor o raciocínio por trás das mudanças se eles existissem quando você fez a alteração.

Além disso, supondo que você esteja fazendo TDD ou alguma forma de refator verde vermelho, uma maneira de garantir que você obtenha o envolvimento de seus colegas é usar a técnica de emparelhamento de ping pong. Simplesmente explicado à medida que o driver é girado para cada etapa do ciclo RGR, ou seja, o par 1 grava um teste com falha, o par 2 o corrige, o par 1 refatores, o par 2 escreve um teste com falha ... e assim por diante.

narcisos
fonte
Excelentes pontos. Infelizmente, sinceramente duvido que seja capaz de mudar "o sistema". Às vezes, os revisores também são de diferentes fusos horários e localizações geográficas; portanto, nesses casos, ele não voará independentemente.
t0x1n 12/06
6

Provavelmente, esse problema reflete um problema muito maior com a cultura da organização. As pessoas parecem mais interessadas em fazer "seu trabalho" do que em melhorar o produto, provavelmente esta empresa tem uma cultura de "culpa" em vez de uma cultura colaborativa e as pessoas parecem mais interessadas em se cobrir do que em ter uma visão completa do produto / empresa .

Na minha opinião, você está completamente certo, as pessoas que revisam seu código estão completamente erradas se elas reclamarem porque você "toca" coisas fora de "sua tarefa", tenta convencer essas pessoas, mas nunca se opõe a seus princípios, para mim isso é a qualidade mais importante de um verdadeiro profissional.

E se a coisa certa dá a você números ruins de alguma maneira estúpida da corporação para avaliar seu trabalho, qual é o problema ?, quem quer "vencer" esse jogo de avaliação em uma empresa insana ?, tente mudá-lo! você está cansado, encontra outro lugar, mas nunca, nunca seja contra seus princípios, é o melhor que você tem.

AlfredoCasado
fonte
11
Você começa a querer ganhar o jogo de avaliação uma vez que você perceber que recompensa com aumentos salariais, bônus e opções de ações :)
t0x1n
2
Não. Você está trocando dinheiro por princípios @ t0x1n. Simples assim. Você tem três opções: trabalhar para consertar o sistema, aceitar o sistema, sair do sistema. As opções 1 e 3 são boas para a alma.
Sean Perry
2
não apenas é ruim para a alma, uma empresa com princípios que se concentram nas máximas locais é normalmente menos ideal do que uma empresa que se concentra nas máximas globais. Não apenas isso, trabalhar não é apenas dinheiro, você gasta muito tempo todos os dias em seu trabalho, se sente confortável em seu trabalho, sente que está fazendo as coisas certas, provavelmente é muito melhor que um monte de dólares a cada mês.
AlfredoCasado
11
Meh, as almas são superestimadas;)
t0x1n
2
@SeanPerry Eu realmente tentei consertar o sistema, mas foi muito difícil. (1) Eu estava praticamente sozinho nisso, e é difícil ir contra os grandes chefes (eu sou apenas um desenvolvedor regular, nem mesmo sênior). (2) Essas coisas levam tempo que eu simplesmente não tinha. Há muito trabalho e todo o ambiente consome muito tempo (e-mails, interrupções, CRs, ciclos de teste com falha que você precisa corrigir, reuniões, etc. etc.). Eu faço o meu melhor para filtrar e ser produtivo, mas normalmente eu mal posso terminar a minha "prescrito" trabalho em tempo (com elevados padrões não ajuda aqui), deixar o trabalho sozinho em mudar o sistema ...
t0x1n
5

Às vezes, refatorar é uma coisa ruim. Não pelas razões que seus revisores de código estão dando; parece que eles realmente não se preocupam com a qualidade do código, e você fazê- cuidado, que é impressionante. Mas há dois motivos que devem impedir você de refatorar : 1. Você não pode testar as alterações feitas com testes automatizados (testes de unidade ou o que for) ou 2. Você está fazendo alterações em algum código que não entende muito bem; ou seja, você não possui o conhecimento específico do domínio para saber quais alterações deve fazer.

1. Não refatorar quando não puder testar as alterações feitas com testes automatizados.

A pessoa que está revisando seu código precisa ter certeza de que as alterações feitas não quebraram nada do que costumava funcionar. Esse é o maior motivo para deixar o código ativo em paz. Sim, definitivamente seria melhor refatorar essa função de 1000 linhas de comprimento (que realmente é uma abominação!) Em várias funções, mas se você não pode automatizar seus testes, pode ser muito difícil convencer os outros de que você fez tudo direito. Eu definitivamente cometi esse erro antes.

Antes de refatorar, verifique se há testes de unidade. Se não houver testes de unidade, escreva alguns! Depois refatorar, e seus revisores de código não terão desculpa (legítima) por ficarem chateados.

Não refatorar trechos de código que exijam conhecimento específico do domínio que você não possui.

O local em que trabalho tem muito código pesado de engenharia química. A base de código usa os idiomas comuns aos engenheiros químicos. Nunca faça alterações idiomáticas em um campo. Pode parecer uma ótima idéia para mudar o nome de uma variável chamada xpara molarConcentration, mas adivinhem? Em todos os textos de química, a concentração molar é representada com um x. A renomeação torna mais difícil saber o que realmente está acontecendo no código para as pessoas desse campo.

Em vez de renomear variáveis ​​idiomáticas, basta colocar comentários explicando o que são. Se eles são não idiomática, por favor, renomeá-los. Não deixe a i, j, k, x, y, etc. flutuante em torno de quando nomes melhores funcionará.

Regra geral para abreviações: se, quando as pessoas estão falando, elas usam uma abreviação, não há problema em usar essa abreviação no código. Exemplos da base de código no trabalho:

Temos os seguintes conceitos que sempre abreviamos quando falamos sobre eles: "Área de preocupação" se torna "AOC", "Explosão de nuvem de vapor" se torna VCE, coisas assim. Em nossa base de código, alguém refatorou todas as instâncias chamadas aoc para AreaOfConcern, VCE para vaporCloudExplosion, nPlanes para confiningPlanesCount ... o que tornou o código realmente muito mais difícil de ler para pessoas que tinham conhecimento específico do domínio. Também fui culpado de fazer coisas assim.


Na verdade, isso pode não se aplicar à sua situação, mas existem meus pensamentos sobre o assunto.

Cody Piersall
fonte
Obrigado, Cody. Com relação a "seus revisores de código não terão desculpa (legítima) por ficarem chateados" - a desculpa deles já é ilegítima, pois estão chateados com a dificuldade crescente de revisar as alterações, em vez de correção, legibilidade ou algo assim.
04
2

Agora, essa pergunta tem dois problemas distintos: facilidade de revisar alterações nas revisões de código e tempo gasto na refatoração.

Onde trabalho, há uma prática de desenvolvimento que causa atrito intenso - Revisão de Código (CR). Sempre que mudo algo que não esteja no escopo da minha "tarefa", sou criticado pelos meus revisores por dificultar a revisão.

Como outras respostas disseram - você pode separar os checkins de refatoração dos check-ins de alteração de código (não necessariamente como revisões separadas)? Dependendo da ferramenta que você está usando para fazer a revisão do código, você poderá visualizar as diferenças entre revisões específicas apenas (o Atlassian's Crucible definitivamente faz isso).

(2) Ninguém vai olhar favoravelmente para você liberando CRs "refatorando" que você não deveria fazer. Um CR tem uma certa sobrecarga e seu gerente não quer que você "perca seu tempo" em refatoração. Quando está incluído na alteração que você deve fazer, esse problema é minimizado.

Se a refatoração for simples e facilitar a compreensão do código (que é tudo o que você deve fazer se for apenas refatoração), a revisão do código não deve demorar muito para ser concluída e deve ser uma sobrecarga mínima que é recuperada quando alguém precisa vir e olhar o código novamente no futuro. Se seus chefes não conseguem entender isso, talvez seja necessário incentivá-los gentilmente a alguns recursos que discutem por que a regra dos escoteiros leva a um relacionamento mais produtivo com seu código. Se você puder convencê-los de que o "desperdício de seu tempo" agora economizará duas, cinco ou dez vezes mais tarde, quando você / outra pessoa voltar para fazer mais trabalho nesse código, seu problema deverá desaparecer.

O problema é exacerbado pelo Resharper, pois cada novo arquivo que adiciono à alteração (e não posso saber com antecedência exatamente quais arquivos acabariam sendo alterados) geralmente está repleto de erros e sugestões - a maioria dos quais está no local e merece totalmente fixação.

Você já tentou levar essas questões à atenção de seus colegas e discutir por que elas valem a pena ser corrigidas? E algum deles pode ser corrigido automaticamente (supondo que você tenha cobertura de teste suficiente para confirmar que não quebrou as coisas com a refatoração automática)? Às vezes, não vale a pena fazer uma refatoração, se é algo realmente exigente. Sua equipe inteira usa o ReSharper? Em caso afirmativo, você possui uma política compartilhada sobre quais avisos / regras estão sendo aplicados? Caso contrário, talvez você deva demonstrar onde a ferramenta está ajudando a identificar áreas do código que são possíveis fontes de sofrimento futuro.

Chris Cooper
fonte
Em relação à separação CR, apontei no meu post e vários outros comentários por que acho isso impossível.
t0x1n 12/06
Não estou falando de coisas exigentes, estou falando de problemas reais de desempenho e correção, além de código redundante, código duplicado etc. . Infelizmente, nem todos da minha equipe usam o recarregador, e mesmo aqueles que não levam isso muito a sério. É necessário um grande esforço educacional, e talvez eu tente liderar algo assim. É difícil, como eu mal tenho tempo suficiente para o trabalho que tenho que fazer, muito menos para projetos de educação paralelos. Talvez eu só precise esperar um período de inatividade para explorar.
t0x1n 12/06
Eu só vou gritar e dizer que definitivamente não é impossível , como eu vejo isso o tempo todo. Faça a alteração que você faria sem a refatoração, faça o check-in e refatore a limpeza e faça o check-in. Não é ciência do foguete. Ah, e esteja preparado para defender por que você acha que vale a pena passar o tempo refatorando e revisando o código refatorado.
Eric King
@ EricKing Suponho que eu poderia fazer isso. No entanto: (1) eu teria que trabalhar com código feio e fazer anotações do que eu quero melhorar até que eu termine com as alterações funcionais que sugam e atrasam meu progresso funcional (2) Depois de enviar minhas alterações funcionais e revisite minhas anotações para refatoração, que será apenas a primeira iteração e a conclusão da refatoração poderá levar mais tempo, o que, como você sugeriu, seria difícil explicar aos meus gerentes, visto que meu trabalho já está "concluído".
t0x1n 12/06
2
"Estou falando de problemas reais de desempenho e correção" - isso pode estar estendendo a definição de refatoração; se o código estiver realmente incorreto, isso constituiria correção de bugs. Quanto a problemas de desempenho, isso não é algo que você deve consertar apenas como parte de uma alteração de recurso, provavelmente é algo que precisa de medição, teste completo e revisão de código separada.
Chris Cooper
2

Lembro-me de 25 anos atrás, "limpando" o código no qual eu estava trabalhando para outros fins, principalmente reformatando blocos de comentários e alinhamentos de guias / colunas para tornar o código puro e, portanto, mais fácil de entender (sem alterações funcionais reais) . Acontece que eu gosto de código limpo e bem ordenado. Bem, os desenvolvedores seniores ficaram furiosos. Acontece que eles usaram algum tipo de comparação de arquivo (diff) para ver o que havia mudado, em comparação com suas cópias pessoais do arquivo, e agora estava dando todos os tipos de falsos positivos. Devo mencionar que nossa biblioteca de códigos estava em um mainframe e não tinha controle de versão - você basicamente substituiu o que estava lá, e foi isso.

A moral da história? Não sei. Acho que às vezes você não pode agradar a ninguém, exceto a si mesmo. Eu não estava perdendo tempo (aos meus olhos) - o código limpo era mais fácil de ler e entender. É que as ferramentas primitivas de controle de alterações usadas por outras pessoas colocam algum trabalho extra de uma só vez. As ferramentas eram muito primitivas para ignorar espaço / tabulação e comentários atualizados.

Phil Perry
fonte
Sim, eu também sou dispensado por espaçamento, bem como coisas triviais, como elenco redundante, etc. O que não é completamente obrigatório pela minha mudança é "ruído".
t0x1n
2

Se você pode dividir a alteração solicitada e o refatoramento não solicitado em (muitas) confirmações separadas, conforme observado por @Useless, @Telastyn e outros, é o melhor que você pode fazer. Você ainda estará trabalhando em um único CR, sem a sobrecarga administrativa de criar um "refatorador". Apenas mantenha seus commits pequenos e focados.

Em vez de dar alguns conselhos sobre como fazê-lo, prefiro apontar uma explicação muito maior (de fato, um livro) de um especialista. Dessa forma, você poderá aprender muito mais. Leia Trabalhando efetivamente com o código herdado (de Michael Feathers) . Este livro pode ensiná-lo a fazer a refatoração, intercalada com as mudanças funcionais.

Os tópicos abordados incluem:

  • Entendendo a mecânica da mudança de software: adicionando recursos, corrigindo bugs, melhorando o design, otimizando o desempenho
  • Colocando código legado em um equipamento de teste
  • Escrever testes que protegem você contra a introdução de novos problemas
  • Técnicas que podem ser usadas com qualquer linguagem ou plataforma - com exemplos em Java, C ++, C e C #
  • Identificando com precisão onde as alterações de código precisam ser feitas
  • Lidar com sistemas legados que não são orientados a objetos
  • Manipulação de aplicativos que parecem não ter estrutura

Este livro também inclui um catálogo de vinte e quatro técnicas de quebra de dependência que o ajudam a trabalhar com elementos do programa isoladamente e a fazer alterações mais seguras.

Hbas
fonte
2

Eu também acredito muito na regra dos escoteiros e na refatoração oportunista. O problema geralmente está em conseguir a participação da gerência. A refatoração vem com riscos e custos. O que a administração geralmente ignora é que o mesmo ocorre com a dívida técnica.

É da natureza humana. Estamos acostumados a lidar com problemas reais, sem tentar evitá-los. Somos reativos, não pró-ativos.

O software é intangível e, portanto, é difícil entender que, como um carro, ele precisa de manutenção. Nenhuma pessoa razoável compraria um carro e nunca o repararia. Aceitamos que essa negligência diminuiria sua longevidade e, a longo prazo, seria mais cara.

Apesar de muitos gerentes entenderem isso, eles são responsabilizados por alterações no código de produção. Existem políticas que tornam mais fácil deixar bem o suficiente. Muitas vezes, a pessoa que precisa convencer está realmente acima do seu gerente e simplesmente não quer lutar para colocar suas "grandes idéias" em produção.

Para ser sincero, seu gerente nem sempre está convencido de que seu remédio é, de fato, tão bom quanto você pensa. (Óleo de cobra?) Há vendas. É seu trabalho ajudá-lo a ver o benefício.

A gerência não compartilha suas prioridades. Coloque seu chapéu de gerenciamento e veja com os olhos dele. Você provavelmente encontrará o ângulo certo. Seja persistente. Você efetuará mais alterações ao não permitir que a primeira resposta negativa o impeça.

Mario T. Lanza
fonte
1

Se você pode dividir a mudança solicitada e o refatoramento não solicitado em duas solicitações de mudança separadas, conforme observado por @ GlenH7, é o melhor que você pode fazer. Você não é "o cara que perde tempo", mas "o cara que trabalha duas vezes mais".

Se você se encontra frequentemente na situação em que não pode dividi-los, porque o trabalho solicitado agora precisa do refator não solicitado para compilar, sugiro que você insista em ter ferramentas para verificar automaticamente os padrões de codificação, usando os argumentos descritos aqui (o Resharper os avisos por si só podem ser um bom candidato, não estou familiarizado com isso). Esses argumentos são todos válidos, mas há um extra que você pode usar para sua vantagem: ter esses testes agora faz com que seja seu dever passar nos testes em cada confirmação.

Se sua empresa não deseja "perder tempo refatorando" (um sinal ruim da parte deles), deve fornecer um arquivo de "supressão de aviso" (cada ferramenta possui seu próprio mecanismo) para que você não fique mais irritado com tais avisos. Digo isso para fornecer opções para diferentes cenários, até no pior dos casos. Também é melhor ter acordos claramente definidos entre você e sua empresa, também sobre a qualidade esperada do código, em vez de suposições implícitas de cada lado.

logc
fonte
Essa seria uma boa sugestão para um novo projeto. No entanto, nossa base de código atual é enorme e o Resharper emite muitos erros para a maioria dos arquivos. É simplesmente tarde demais para aplicá-lo, e suprimir erros existentes torna ainda pior - você sentirá falta deles no seu novo código. Além disso, existem muitos erros, problemas e cheiros de código que um analisador estático não captura, eu estava apenas dando avisos ao Resharper como exemplo. Novamente, eu posso ter escrito a "parte desperdiçada" de maneira um tanto severa, eu deveria ter dito algo como "tempo dedicado a algo que não é uma prioridade".
t0x1n 12/06
@ t0x1n: a regra dos escoteiros envolve principalmente os módulos que você está tocando. Isso pode ajudá-lo a desenhar uma primeira linha divisória. Substituir avisos não é uma boa ideia, eu sei, mas, da perspectiva da empresa, suprimi-los em um novo código está correto e seguindo suas convenções - bem, talvez eu esteja me deixando levar pela minha própria argumentação :)
logc
11
essa é a parte frustrante! Eu apenas toco em arquivos que eu teria tocado de qualquer maneira como parte da minha tarefa, mas estou recebendo reclamações da mesma forma! Os avisos de que estou falando são aviso não estilo, eu estou falando sobre o desempenho real e questões de correção, bem como código redundante, código duplicado, etc.
t0x1n
@ t0x1n: parece realmente muito frustrante. Observe que eu não quis dizer apenas "análise de código estática", mas também outras recomendações, algo equivalente ao Nexus. Obviamente, nenhuma ferramenta captura 100% semântica; isso é apenas uma estratégia de remediação.
logc 12/06