Estou reunindo algumas diretrizes para revisões de código. Ainda não temos um processo formal e estamos tentando formalizá-lo. E nossa equipe é distribuída geograficamente.
Estamos usando o TFS para controle de origem (também para tarefas / rastreamento de bugs / gerenciamento de projetos, mas foi migrado para o JIRA ) com o Visual Studio 2008 para desenvolvimento.
Quais são as coisas que você procura ao fazer uma revisão de código?
- Essas são as coisas que inventei
- Aplicar regras FxCop (somos uma loja da Microsoft)
- Verifique se há desempenho (alguma ferramenta?) E segurança (pensando em usar OWASP - rastreador de código ) e segurança de encadeamento
- Siga as convenções de nomenclatura
- O código deve cobrir casos de borda e condições de contorno
- Deve lidar com exceções corretamente (não engula exceções)
- Verifique se a funcionalidade está duplicada em outro lugar
- Um corpo de método deve ser pequeno (20 a 30 linhas) e os métodos devem fazer uma coisa e apenas uma coisa (sem efeitos colaterais e evitar acoplamento temporal)
- Não passar / retornar nulos nos métodos
- Evite código morto
- Documentar métodos / propriedades / variáveis públicos e protegidos
Que outras coisas devemos procurar em geral?
Estou tentando ver se podemos quantificar o processo de revisão (ele produziria saída idêntica quando revisado por pessoas diferentes) Exemplo: Dizendo "o corpo do método não deve ter mais que 20 a 30 linhas de código", em vez de dizer "o método o corpo deve ser pequeno ".
Ou a revisão de código é muito subjetiva (e seria diferente de um revisor para outro)?
O objetivo é ter um sistema de marcação (digamos -1 ponto para cada violação da regra do FxCop, -2 pontos por não seguir as convenções de nomenclatura, 2 pontos por refatoração etc.) para que os desenvolvedores tenham mais cuidado ao verificar seu código. Dessa forma, podemos identificar desenvolvedores que estão constantemente escrevendo código bom / ruim. O objetivo é fazer com que o revisor gaste cerca de 30 minutos no máximo, para fazer uma revisão (eu sei que isso é subjetivo, considerando o fato de que o conjunto de alterações / revisão pode incluir vários arquivos / grandes mudanças na arquitetura existente etc.), mas você obtém a ideia geral, o revisor não deve passar dias revisando o código de alguém).
Que outro sistema objetivo / quantificável você segue para identificar o código bom / ruim escrito pelos desenvolvedores?
Referência do livro: Código Limpo: Um manual de artesanato em software ágil de Robert Martin .
fonte
this.Foo().FooBar().FooBarBar();Se o objeto retornado aqui do Foo for nulo, você definitivamente poderá evitar "Referência de objeto não definida como uma instância de um objeto" ao chamar FooBar ()Respostas:
Classificar indivíduos em uma revisão é contrário aos sistemas mais bem-sucedidos com os quais trabalhei, talvez todos. Mas o objetivo que tenho tentado alcançar há mais de 20 anos é menos bugs e maior produtividade por hora de engenheiro. Se a classificação de indivíduos é uma meta, suponho que as revisões possam ser usadas. Nunca vi uma situação em que isso fosse necessário, como trabalhador ou líder.
Alguns estudos objetivos (Fagan, etc.) e muita sabedoria popular sugerem que os relacionamentos entre pares facilitam as revisões de código destinadas a reduzir bugs e aumentar a produtividade. Os gerentes de trabalho podem participar como trabalhadores, mas não como gerentes. Os pontos de discussão são observados, as mudanças para satisfazer os revisores geralmente são boas, mas não são necessárias. Daí a relação entre pares.
Quaisquer ferramentas automatizadas que possam ser aceitas sem análises ou julgamentos adicionais são úteis em C, C ++, Java. Compilação regular. Compiladores são realmente bons em encontrar erros de compilador. A documentação de desvios nas verificações automatizadas soa como uma acusação sutil das verificações automatizadas. Diretivas de código (como Java faz) que permitem desvios são bastante perigosos, IMHO. Ótimo para depuração, para permitir que você entenda o assunto rapidamente. Não é tão bom encontrar em um bloco de código com menos de 50.000 linhas de comentários e mal documentado pelo qual você se tornou responsável.
Algumas regras são estúpidas, mas fáceis de aplicar; padrões para cada instrução switch, mesmo quando inacessíveis, por exemplo. Então é apenas uma caixa de seleção e você não precisa gastar tempo e dinheiro testando valores que não correspondem a nada. Se você tiver regras , terá insensatez , elas são indissociáveis . O benefício de qualquer regra deve valer a tolice que custa, e esse relacionamento deve ser verificado em intervalos regulares.
Por outro lado, "Funciona" não é virtude antes da revisão, nem defesa na revisão. Se o desenvolvimento seguiu o modelo em cascata , você gostaria de fazer a revisão quando a codificação estiver 85% concluída, antes que erros complicados sejam encontrados e solucionados, porque a revisão é uma maneira mais barata de encontrá-los. Como a vida real não é o modelo em cascata, quando revisar é algo de uma arte e equivale a uma norma social. As pessoas que realmente lerão seu código e procurarão problemas são ouro sólido. A administração que suporta isso de maneira contínua é uma pérola acima do preço. As revisões devem ser como check-ins, antecipadas e frequentes .
Eu achei essas coisas benéficas:
1) Sem guerras de estilo . Para onde vão os chavetas abertas, deve estar sujeito apenas a uma verificação de consistência em um determinado arquivo. Tudo o mesmo. Tudo bem então. O mesmo vale para a profundidade de recuo ** se ** as larguras de tabulação . A maioria das organizações descobre que precisa de um padrão comum para tabulação, usado como um espaço grande.
2) `Ragged
texto que não
para o conteúdo.`
Aliás, a K&R identificou cinco (CINCO) espaços, portanto, os apelos à autoridade são inúteis. Apenas seja consistente.
3) Uma cópia numerada de linha, inalterável e disponível ao público do arquivo a ser revisado deve ser apontada por 72 horas ou mais antes da revisão.
4) Nenhum projeto on-the-fly. Se houver um problema ou um problema, anote sua localização e siga em frente.
5) O teste que percorre todos os caminhos no ambiente de desenvolvimento é uma idéia muito, muito, muito, muito boa. Testes que exigem dados externos maciços, recursos de hardware, uso do site do cliente etc. etc são testes que custam uma fortuna e não serão completos.
6) Um formato de arquivo não ASCII é aceitável se ferramentas de criação, exibição, edição etc. existirem ou forem criadas no início do desenvolvimento. Esse é um viés pessoal meu, mas em um mundo em que o sistema operacional dominante não pode sair do seu caminho com menos de 1 gigabyte de RAM, não consigo entender por que arquivos com menos de, digamos, 10 megabytes devem ser qualquer coisa diferente de ASCII ou algum outro formato comercialmente suportado. Existem padrões para gráficos, som, filmes, executável e ferramentas que os acompanham. Não há desculpa para um arquivo que contém uma representação binária de algum número de objetos.
Para manutenção, refatoração ou desenvolvimento de código liberado, um grupo de colegas de trabalho que eu havia usado para revisar por outra pessoa, sentado em uma tela e olhando para um diferencial do antigo e do novo , como uma porta de entrada para o check-in da filial. Eu gostei, era barato, rápido, relativamente fácil de fazer. As orientações para pessoas que não leram o código com antecedência podem ser educativas para todos, mas raramente aprimoram o código do desenvolvedor.
Se você está geograficamente distribuído, olhar para as diferenças na tela enquanto conversa com alguém olhando para o mesmo seria relativamente fácil. Isso abrange duas pessoas olhando para mudanças. Para um grupo maior que leu o código em questão, vários sites não são muito mais difíceis do que todos em uma sala. Várias salas ligadas por telas de computador compartilhadas e caixas de proteção funcionam muito bem, IMHO. Quanto mais sites, mais gerenciamento de reuniões é necessário. Um gerente como facilitador pode ganhar seu sustento aqui. Lembre-se de continuar pesquisando os sites em que você não está.
Em um ponto, a mesma organização tinha testes de unidade automatizados que foram usados como testes de regressão. Isso foi muito legal. É claro que mudamos de plataforma e o teste automatizado foi deixado para trás. A revisão é melhor, como observa o Manifesto Ágil , os relacionamentos são mais importantes que processos ou ferramentas . Porém, após a revisão, os testes de unidade / testes de regressão automatizados são a próxima ajuda mais importante na criação de um bom software.
Se você puder basear os testes nos requisitos , bem, como a senhora diz em "Quando Harry conheceu Sally" , terei o que ela está tendo!
Todas as revisões precisam ter um estacionamento para capturar requisitos e problemas de design no nível acima da codificação. Uma vez que algo é reconhecido como pertencente ao estacionamento, a discussão deve parar na revisão.
Às vezes, acho que a revisão de código deve ser como revisões esquemáticas no design de hardware - completamente público, completo, tutorial, o fim de um processo, um gateway após o qual é construído e testado. Porém, as revisões esquemáticas são pesadas porque a alteração de objetos físicos é cara. As revisões de arquitetura, interface e documentação de software provavelmente devem ser pesadas. O código é mais fluido. A revisão de código deve ser mais leve.
De várias maneiras, acho que a tecnologia é tanto sobre cultura e expectativa quanto sobre uma ferramenta específica. Pense em todas as improvisações da " Swiss Family Robinson " / Flintstones / McGyver que encantam o coração e desafiam a mente. Queremos que nossas coisas funcionem . Não existe um único caminho para isso, assim como não havia "inteligência" que de alguma forma pudesse ser abstraída e automatizada pelos programas de IA da década de 1960 .
fonte
A maioria dos pontos que você descreveu é apenas uma questão de formatação de código ou material "superficial":
Tudo isso pode ser verificado usando alguma ferramenta automatizada : não é necessário que um desenvolvedor experiente gaste tempo lendo o código para observar isso.
Não sei nada sobre .NET , mas, para PHP , temos ferramentas para verificar esse tipo de coisa; Considerando que o .NET costuma ser "mais industrial" que o PHP, eu ficaria surpreso ao saber que não há nenhuma ferramenta para verificar esse tipo de coisa.
A ferramenta automatizada pode:
O e-mail pode ser enviado para toda a equipe ou para o cara que cometeu o código que não passa no teste - ou você pode usar alguma interface da Web de relatórios (mesma observação sobre .NET e PHP)
Eu também acrescentaria que o teste automatizado pode ajudar muito, para detectar um certo número de erros antes que o código seja usado na produção. E testes automatizados também podem ajudar com algumas métricas, suponho.
As revisões de código feitas por desenvolvedores experientes também têm outra grande vantagem sobre a qual você não falou:
Mas para uma revisão de código que é mais profunda do que apenas a formatação de código, você precisará de mais de meia hora ...
fonte
Minhas experiências com a revisão de código é que deve ser um esforço combinado para melhorar o código, não uma 'medida' para decidir quem é bom ou ruim em seu trabalho. Quando não importa se você recebe muitas observações durante a revisão do código, os revisores serão mais rigorosos, fornecendo sugestões para melhorar o código.
Para melhorar a qualidade do código verificado, imponha que os comentários de revisão sejam processados (permita que o revisor aprove os comentários processados) e também use ferramentas de verificação de código estático para forçar um nível de qualidade para a confirmação inicial.
fonte
Eu acho que o seu sistema de classificação é uma má ideia. Qual é o ponto? Para identificar bons e maus programadores? Todos na revisão de código podem formar uma avaliação sobre um programador específico com base no código apresentado na revisão de código melhor do que uma atribuição arbitrária de valores a um conjunto de características um tanto arbitrário. Se você deseja identificar bons e maus programadores ... pergunte aos programadores. Garanto que os humanos podem fazer essa avaliação melhor do que sua heurística boba.
Minha sugestão seria tentar melhorar as revisões de código para que as pessoas compartilhem idéias e opiniões abertamente em um ambiente não crítico e hostil. Se você puder fazer isso, estará 100 vezes melhor do que julgar os programadores com base em suas listas de verificação tolas que pretendem fazer um bom trabalho de avaliação de programadores. Eu acho que muitos programadores já se orgulham e são duros consigo mesmos se fazem mal nas revisões de código; Eu questiono se 'punição' adicional por desempenho ruim geralmente é útil.
fonte
Meu único conselho seria evitar tornar o processo de revisão de código muito rigoroso - o mais importante é que a revisão do código realmente ocorra e seja levada a sério .
Quanto mais exaustivo for o processo para o revisor, menor a probabilidade de que as revisões de código ocorram e que sejam levadas a sério, em vez de serem vistas apenas como um aborrecimento. Além disso, o valor real das revisões de código está na capacidade do revisor de usar seu próprio julgamento. Ferramentas automáticas podem ser usadas para verificar itens como se as regras da FXCop passam.
fonte
Como regra geral, evite gastar algum tempo em uma revisão de código fazendo algo que poderia ser feito pela máquina. Por exemplo, seu primeiro item é "impor regras do FxCop", mas presumivelmente isso pode ser feito pelo FxCop sem que os humanos também precisem fazê-lo.
fonte
Se você puder mensurá-lo, se for objetivo, quantificável, tente ter uma ferramenta para fazê-lo. Onde você precisa de um revisor experiente é o material subjetivo confuso.
fonte
Muitos bons comentários já foram feitos sobre questões de estilo, o que é importante. Em um projeto de equipe, é valioso que todo o código pareça ter sido escrito por um único autor. Isso facilita a participação de outros membros da equipe e a correção de problemas quando eles ocorrem. Quais medidas quantitativas você escolhe para garantir esse objetivo mais amplo são menos importantes.
Um item adicional é garantir que o código corresponda à arquitetura geral acordada para o restante do sistema. Problemas semelhantes devem ser resolvidos da mesma maneira. Se a lógica do aplicativo foi dividida em várias camadas, o código que está sendo revisado divide sua funcionalidade da mesma maneira que o resto do sistema? Como alternativa, o código em análise ensina algo novo que deve ser revertido pelo resto do sistema? Assim como as verificações de estilo garantem que o código tenha a mesma aparência, a revisão da arquitetura deve garantir que o código funcione da mesma maneira. A ênfase aqui novamente está na manutenção. Qualquer pessoa da equipe deve ser capaz de entrar neste código e ter uma idéia do que está acontecendo imediatamente.
A idéia de classificação parece um desastre, mas você conhece melhor sua equipe. É possível que eles sejam motivados por esse sistema, mas acho mais provável que as pessoas comecem a se preocupar mais com a nota do que com a solução de problemas. Um dos efeitos colaterais realmente valiosos das análises de código são as oportunidades de orientação que elas oferecem. O revisor deve tratar a pessoa que escreveu o código como alguém que está orientando. Cada problema encontrado não é um problema, mas uma oportunidade de criar um membro da equipe mais experiente e sofisticado e uma equipe mais unida.
fonte
Na verdade, eu me preocupo mais com as coisas "subjetivas" do que qualquer outra coisa, francamente. O que eu quero de uma boa revisão de código é alguém para verificar minha lógica, não minha digitação. E é nisso que eu me concentro quando faço uma revisão de código.
O formato geral que eu gosto de usar é:
Sem isso, apenas olhar as diferenças tende a contribuir com questões menores ou pontos estilísticos. Estou muito mais preocupado em saber se a lógica está correta, se a abordagem geral é boa e se a solução será sustentável.
Como exemplo, recentemente observei algum código de um colega de trabalho. O problema original era uma violação do FxCop. Mas o que estava sendo feito estava tentando determinar a presença ou ausência de um recurso do Windows, verificando o número da versão. Minha principal contribuição foi que essa era uma maneira frágil de fazê-lo, e é melhor consultar o serviço diretamente, pois o mapeamento entre a existência de recursos e o sku do Windows pode mudar no futuro e não é à prova de futuro.
fonte
A complexidade ciclomática (CC) é uma maneira de avaliar o código 'não é ruim'.
No código real que tem CC alto, eu tenho o fator "o que está acontecendo aqui, não me lembro". Um código CC inferior é mais fácil de descobrir.
Obviamente, as advertências usuais aplicam-se a métricas.
fonte
As revisões de código são subjetivas e objetivas. Regras como "o corpo do método deve ter de 20 a 30 linhas" são subjetivas (algumas pessoas podem pensar que 100 linhas são aceitáveis), mas se sua empresa decidiu que 20 a 30 linhas é o limite, tudo bem e você pode medir isso. Eu acho que o sistema de pontos que você criou é uma ótima idéia. Você precisará reavaliar periodicamente, pois descobre que certas regras precisam ter mais ou menos peso na pontuação, mas, desde que todos conheçam as regras, parece um bom sistema.
Outras coisas que eu procuraria:
fonte
Parece que você está ficando detalhado demais rápido demais. Você deveria dividir mais. Você deve observar o código por sua qualidade e conformidade de recursos. Vocês deveriam ter se separado e isso nem é o fim da história ... então aqui está minha sugestão:
Qualidade do código:
Conformidade com os recursos:
Se você pode se cobrir desses dois aspectos de uma revisão de código, você é o melhor.
fonte
Eu poderia escrever alguns parágrafos, mas apenas descreveria o que Karl Wiegers explica em "Revisões por pares em software: um guia prático" . Eu acho que o livro dele contém respostas explícitas e concisas à sua pergunta (e muito mais).
fonte
Depende.
Algumas partes da revisão são facilmente quantificáveis (sem problemas FxCop, há StyleCop erros, há CAT.NET erros, etc.)
O estilo, no entanto, pode ser subjetivo - mas, como você diz, depois de começar a ser mais específico (nenhum método> 20 linhas), você pode medi-lo, e ferramentas como o NDepend podem fazer isso automaticamente. No entanto, algumas coisas nunca serão automáticas - o controle de casos extremos exigiria testes para fazê-lo, o que traz cobertura de código e 100% é um ideal inacessível em muitos casos. A verificação de duplicação é difícil de fazer automaticamente. Verificações nulas, não tenho certeza se concordo com você, mas você pode escrever regras NDepend ou regras FxCop para essa.
Quanto mais ferramentas, melhor e se as ferramentas permitirem que os desenvolvedores verifiquem seu trabalho antes de confirmar as alterações e que as verificações sejam executadas como parte do processo de IC , você minimizará a necessidade de revisões.
fonte
Um sistema de marcação parece difícil de acertar, mas vale a pena ter como ferramenta de medição: você não pode melhorar o que não pode medir. Mas você provavelmente deve aceitar que algumas coisas serão difíceis / impossíveis de quantificar com precisão. O mais complicado é calcular quantos pontos cada qualidade deve pontuar: por exemplo, se a adesão às convenções de nomenclatura tiver 2 pontos, quantos pontos manterão os métodos pequenos?
Talvez algo como uma lista de verificação simples seja melhor, para que o código possa ser marcado como conforme, parcialmente ou não conforme uma qualidade específica. Mais tarde, você pode adicionar pontuação à lista de verificação depois de ver quais problemas de qualidade surgem com mais frequência ou causam mais problemas.
O processo de revisão também deve ser flexível o suficiente para permitir que o código falhe em partes da revisão, desde que isso possa ser justificado e documentado. Aderir cegamente a algum padrão de qualidade de código que faz com que um componente se torne desnecessariamente complexo / incontrolável é uma má ideia!
fonte
Se você deseja que o código das pessoas seja mais padronizado, sem fazê-las "perder tempo com a formatação", como alguns desenvolvedores reclamarão. Invista em uma ferramenta como o ReSharper , pois torna a correção da formatação e outras tarefas de re-fatoração um processo quase automatizado.
fonte
fonte