Revue de code: expérience réussie

Il y a une tonne d'informations sur Internet sur les revues de code:

• comment un examen du code de quelqu'un d'autre affecte la culture de l'entreprise
• Contrôles de sécurité réglementaires
• manuels abrégés
• longues listes de recommandations
• revue de code d'un point de vue interpersonnel
• pourquoi ai-je besoin d'une révision du code
• méthodes éprouvées
• en savoir plus sur les méthodes éprouvées
• statistiques: dans quelle mesure la révision du code aide à détecter les bogues
• exemples d'erreurs commises lors de la révision du code

Oui, bien sûr, il existe des livres sur ce sujet. En un mot, cet article décrit comment la révision du code est organisée par Palantir. Dans les organisations dont la culture n'accepte pas un tel examen par les pairs, il peut être utile de lire d'abord le brillant essai de Karl E. Wiegers, The Code Revision with a Human Face, puis d'essayer de suivre ce guide.

Ce texte est extrait des recommandations d'amélioration de la qualité basées sur le travail avec Baseline , notre outil de contrôle qualité du code Java. Il couvre les sujets suivants:

• Pourquoi, quoi et quand essayons-nous de réaliser la révision du code
  
• Préparation du code pour révision
  
• Exécution de la revue de code
  
• Exemples de révision de code
  

Traduit en Alconost


XKCD Comic 'Code Quality', copié sous licence CC BY-NC 2.5

La motivation

Nous sommes engagés dans la révision du code (RC) pour améliorer la qualité du code et voulons ainsi influencer positivement la culture d'équipe et d'entreprise. Par exemple:

• Les auteurs du code sont motivés car ils savent que leur équipe de relecteurs tiendra compte de leur demande de changement: l'auteur essaie de nettoyer la rugosité, de suivre le plan d'action et d'améliorer globalement l'ensemble du commit. Lorsque des collègues reconnaissent votre professionnalisme dans l'écriture de code - pour de nombreux programmeurs, c'est l'occasion d'être fiers d'eux-mêmes.
  
• Le partage des connaissances aide l'équipe de développement de plusieurs manières:
  

- Dans le cadre du RK, il est clairement indiqué quelle partie de la fonctionnalité a été ajoutée / modifiée / supprimée afin qu'à l'avenir, les membres de l'équipe puissent compter sur le travail déjà effectué.

- L'auteur du code peut utiliser une technique ou un algorithme sur lequel les examinateurs eux-mêmes apprendront quelque chose. Dans l'ensemble, une révision du code aide à élever la barre de la qualité dans toute l'organisation .

- Les examinateurs peuvent connaître les techniques de programmation ou la base de code elle-même qui permettront d'améliorer ou de consolider les modifications apportées; par exemple, quelqu'un peut en même temps développer ou modifier exactement la même opportunité.

- Un contact et une communication positifs renforcent les liens sociaux entre les membres de l'équipe.

• En raison de la cohérence de la base de code, le code lui-même est beaucoup plus facile à lire et à comprendre. Il est plus facile de prévenir les bogues et de promouvoir la collaboration entre les programmeurs installés et nomades.
  
• Il est difficile pour l'auteur du code de juger de la lisibilité de sa propre création, et pour le réviseur, c'est juste facile, car il ne voit pas dans quel contexte se situe ce code. Le code lisible par l'homme est plus facile à réutiliser, il contient moins de bogues et est plus durable.
  
• Les erreurs aléatoires (par exemple les fautes de frappe), ainsi que les erreurs structurelles (par exemple le code non exécutable, les erreurs de logique ou d'algorithmes, les problèmes de performances et d'architecture) sont souvent beaucoup plus faciles à détecter par un critique pointilleux qui regarde le code de côté. Selon la recherche, même une revue de code courte et non officielle affecte de manière significative la qualité du code et l'occurrence de bogues .
  
• Les exigences de conformité et les cadres réglementaires nécessitent souvent des examens obligatoires. Un examen du code est un excellent moyen d'éviter les problèmes de sécurité courants. Si les exigences de sécurité dans cet environnement ou par rapport à cette opportunité sont augmentées, alors l'examen sera recommandé (ou même demandé) par des satrapes des autorités réglementaires (un bon exemple est le guide OWASP ).

Que chercher

Il n'y a pas de réponse claire à cette question et chaque équipe de développement doit se mettre d'accord sur sa propre approche. Certaines équipes préfèrent vérifier les éventuelles modifications apportées à la branche principale de développement, tandis que d'autres prennent en compte le «seuil de trivialité», au-delà duquel la vérification est obligatoire. Dans ce cas, vous devez trouver un équilibre entre l'utilisation efficace du temps du programmeur (à la fois l'auteur et le réviseur de code) et le maintien de la qualité du code. Dans certains contextes stricts, il est parfois nécessaire de tout vérifier, même les modifications les plus triviales, dans le code.

Les règles de révision du code sont les mêmes pour tout le monde: même si vous êtes le développeur le plus expérimenté de l'équipe, cela ne signifie pas que votre code ne nécessite pas de révision. Même si (parfois cela arrive) le code est parfait, la revue ouvre des opportunités de mentorat et de coopération, et, au moins, aide à regarder la base de code sous différents points de vue.

Quand vérifier

Un examen du code devrait avoir lieu après la réussite des vérifications automatiques (tests, conception, autres étapes de l'intégration continue), mais avant la fusion du nouveau code avec la branche principale du référentiel. Habituellement, nous n'avons pas recours à une vérification formelle de l'ensemble des modifications apportées au code depuis la dernière version.

Pour les modifications complexes qui doivent être ajoutées à la branche principale du code en tant qu'unité unique, mais si étendues qu'elles ne peuvent pas réellement être couvertes en une seule vérification, essayez une révision en plusieurs phases. Nous créons la branche principale / grande fonctionnalité et un certain nombre de branches secondaires (par exemple, fonctionnalité / grande fonctionnalité-api, fonctionnalité / grande fonctionnalité-test, etc.), chacune d'entre elles encapsulant un sous-ensemble de la fonctionnalité commune, et chacune de ces branches vérifié par rapport à la branche principale de fonctionnalité / grande fonctionnalité. Une fois que toutes les branches secondaires ont été fusionnées en fonction / grande fonctionnalité, créez une revue de code pour injecter cette dernière dans la branche principale.

Préparation du code pour révision

L'auteur du code est tenu de fournir le code de la revue sous une forme digestible, afin de ne pas prendre de temps supplémentaire aux relecteurs et de ne pas les démotiver:

• Portée et taille . Les changements doivent porter sur un champ d'application étroit, bien défini et autosuffisant, entièrement couvert par l'examen. Par exemple, les modifications peuvent être liées à l'implémentation de certaines fonctionnalités ou corrections de bogues. De brefs changements sont préférables aux longs. Si la revue contient du matériel qui inclut des modifications dans plus de ~ 5 fichiers, ou nécessite plus de 1 à 2 jours pour l'enregistrement, ou si la revue elle-même peut prendre plus de 20 minutes, essayez de diviser le matériel en plusieurs fragments autonomes et de les vérifier séparément. Par exemple, un développeur peut soumettre une modification qui définit l'API d'une nouvelle fonctionnalité en termes d'interfaces et de documentation, puis ajouter une deuxième modification qui décrit les implémentations de ces interfaces.
  
• Vous devez envoyer uniquement du matériel complet, testé indépendamment (utilisez diff) et testé indépendamment . Pour gagner du temps sur le réviseur, testez les modifications soumises (c'est-à-dire exécutez la suite de tests) et assurez-vous que le code passe tous les assemblages, ainsi que tous les tests et toutes les étapes du contrôle qualité, à la fois localement et sur des serveurs d'intégration continue, puis sélectionnez uniquement examinateurs .
  
• La refactorisation des changements ne devrait pas affecter le comportement et vice versa; les changements qui affectent le comportement ne doivent pas impliquer une refactorisation ou un reformatage du code. Il y a plusieurs bonnes raisons à cela:
  

- Les modifications liées au refactoring affectent généralement de nombreuses lignes de code dans de nombreux fichiers - par conséquent, ce code ne sera pas vérifié avec autant de soin . Des changements de comportement imprévus peuvent s'infiltrer dans la base de code, et personne ne le remarquera même.

- Les changements majeurs associés au refactoring violent les mécanismes de mouvement, de sélection et autres "magie" associés au contrôle de source. Il est très difficile d'annuler un tel changement de comportement qui a été effectué après l'achèvement de la refactorisation qui couvrait l'ensemble du référentiel.

- Le temps de travail coûteux qu'une personne consacre à la révision du code devrait aller vérifier la logique du programme, et non les différends concernant le style, la syntaxe ou la mise en forme du code. Nous préférons résoudre ces problèmes à l'aide d'outils automatisés, en particulier, Checkstyle , TSLint , Baseline , Prettier , etc.

Valider les messages

Voici un exemple de message de validation compétent, conçu conformément à la norme largement utilisée:

 ( 80 )    ,  ,   .   72 .          ,    —   .   ,       (   );     ,  rebase,    .  -   : « »,   « »   « ».      -,  , ,  git merge  git revert.      .    . 

Essayez de décrire les modifications apportées lors de la validation, et comment exactement ces modifications sont effectuées:

 > .   .    . > .  jcsv-     IntelliJ 

Rechercher des réviseurs

Généralement, l'auteur d'une validation recherche un ou deux réviseurs qui connaissent la base de code. Souvent, à ce titre, le chef de projet ou l'ingénieur principal. Il est conseillé au propriétaire du projet de s'abonner à son propre projet afin de recevoir des notifications de nouvelles révisions de code. Avec la participation de trois réviseurs ou plus, une révision du code s'avère souvent improductive ou contre-productive, car différents réviseurs peuvent proposer des changements mutuellement contradictoires. Cela peut indiquer un désaccord fondamental sur la mise en œuvre correcte, et de tels problèmes ne devraient pas être résolus lors de la révision du code, mais lors d'une réunion prolongée à laquelle toutes les parties intéressées participent en personne ou en mode vidéoconférence.

Exécution de la revue de code

Un examen de code est un point de synchronisation entre différents membres de l'équipe, il peut donc potentiellement arrêter le travail. Par conséquent, la révision du code doit être opérationnelle (prendre environ plusieurs heures, pas plusieurs jours), et les membres de l'équipe et les dirigeants doivent être conscients du temps qu'il faudra pour vérifier et prioriser le temps qui lui est alloué en conséquence. S'il vous semble que vous n'avez pas le temps de terminer la révision à temps, informez-en immédiatement l'auteur du commit afin qu'il puisse vous trouver un remplaçant.

La révision doit être assez approfondie pour que le réviseur puisse expliquer le changement à un autre développeur avec suffisamment de détails. Nous veillerons donc à ce que les détails de la base de code soient connus d'au moins deux, et non d'un seul.

En tant que réviseur, vous devez respecter les normes de qualité du code et le maintenir à un niveau élevé. Une révision de code est plus un art qu'une science. Cela ne peut être appris qu'en pratique. Un réviseur expérimenté doit d'abord essayer de laisser ses collègues moins expérimentés changer et les laisser être les premiers à réviser. Si l'auteur a suivi les règles ci-dessus (en particulier celles liées à l'auto-test et à l'exécution préliminaire du code), alors c'est ce que le réviseur doit faire attention lors de la révision:

But

• Le code atteint-il l'objectif fixé par l'auteur? Tout changement doit avoir une raison spécifique (nouvelle fonctionnalité, refactoring, correction de bug, etc.). Le code proposé nous conduit-il vraiment à cet objectif?
  
• Posez des questions. Les fonctions et les classes doivent être justifiées. Si leur affectation au réviseur n'est pas claire, cela signifie peut-être que le code devrait être réécrit ou soutenu par des commentaires ou des tests.
  

Implémentation

• Réfléchissez à la façon dont vous résoudriez ce problème. Sinon, pourquoi? Votre code gère-t-il davantage de cas (limites)? Peut-être que c'est plus court / plus facile / plus propre / plus rapide / plus sûr, et fonctionnellement pas pire? Peut-être avez-vous remarqué une régularité profonde qui n'est pas couverte par le code existant?
  
• Voyez-vous la possibilité de créer des abstractions utiles? Si le code est partiellement dupliqué, cela signifie souvent qu'un élément plus abstrait ou général de la fonction peut en être extrait, qui peut ensuite être réutilisé dans d'autres contextes.
  
• Pensez comme un adversaire, cependant, restez gentil. Essayez de «rattraper» les auteurs qui ont coupé des coins ou qui manquent des cas spécifiques en lançant des configurations / données d'entrée problématiques qui cassent leur code.
  
• Pensez aux bibliothèques ou au code de travail prêt à l'emploi. Si quelqu'un réimplémente la fonctionnalité existante - ce n'est pas seulement parce qu'il ne connaît pas l'existence d'une solution toute faite. Parfois, du code ou des fonctionnalités sont intentionnellement réinventés - par exemple, pour éviter les dépendances. Dans ce cas, cela doit être clairement commenté dans le code. Cette fonctionnalité est-elle déjà fournie dans une bibliothèque existante?
  
• Le changement correspond-il aux modèles standard? Dans les bases de code existantes, des régularités sont souvent tracées liées aux conventions de dénomination, à la décomposition de la logique du programme, aux définitions des types de données, etc. Il est généralement souhaitable que toutes les modifications soient mises en œuvre conformément aux modèles existants.
  
• Des dépendances qui surviennent lors de la compilation ou lors de l'exécution (en particulier entre les sous-projets) sont-elles ajoutées lors d'une modification? Nous recherchons la faible cohérence du code de nos produits, c'est-à-dire que nous essayons de permettre un minimum de dépendances. Les modifications liées aux dépendances et au système de génération doivent être vérifiées particulièrement attentivement.
  

Lisibilité et style

• Pensez à la façon dont vous lisez ce code. Pouvez-vous saisir ses concepts assez rapidement? Est-il raisonnablement présenté, est-il facile de garder une trace des noms des variables et des méthodes? Puis-je tracer du code sur plusieurs fichiers ou fonctions? Avez-vous déjà été dérangé par un nom incohérent quelque part?
  
• Le code est-il conforme aux directives de programmation et de style bien connues? Le code sort-il de l'ensemble du projet par style, conventions de dénomination d'API, etc.? Comme mentionné ci-dessus, nous essayons de résoudre les différends stylistiques à l'aide d'outils automatiques.
  
• Le code a-t-il des listes de tâches (TODO)? Les listes de tâches s'accumulent simplement dans le code et finissent par se transformer en ballast. Attribuez à l'auteur de soumettre un ticket à GitHub Issues ou JIRA et associez un numéro de tâche à TODO. Il ne devrait pas y avoir de code commenté dans la modification proposée.
  

Support d'utilisation

• Lisez les tests. S'il n'y a pas de tests dans le code, mais qu'ils devraient être là, demandez à l'auteur d'en écrire quelques-uns. Les fonctionnalités vraiment non testées sont rares, et les implémentations de fonctionnalités non testées - malheureusement, souvent. Vérifiez les tests eux-mêmes: couvrent-ils des cas intéressants? Est-il pratique de les lire? La couverture globale du test diminue-t-elle après ce test? Réfléchissez à la façon dont ce code pourrait échouer. Les normes de conception de test et le code de base sont souvent différents, mais les normes de test sont également importantes.
  
• L'examen de ce fragment présente-t-il un risque de rupture du code de test, du code d'effraction ou des tests d'intégration? Souvent, un tel examen n'est pas effectué avant la validation / fusion, mais si de tels cas sont négligés, tout le monde en souffrira. Faites attention aux choses suivantes: suppression des utilitaires ou modes de test, modification de la configuration, modifications de la disposition / structure de l'artefact.
  
• Ce changement viole-t-il la compatibilité descendante? Dans l'affirmative, est-il possible d'apporter cette modification à la version à ce stade, ou doit-elle être reportée à une version ultérieure? Les violations peuvent inclure des modifications de la base de données ou de son schéma, des modifications des API publiques, des modifications du flux de tâches au niveau de l'utilisateur, etc.
  
• Ce code nécessite-t-il des tests d'intégration? Parfois, le code ne peut pas être correctement vérifié en utilisant uniquement des tests unitaires, surtout s'il interagit avec des systèmes ou une configuration externes.
  
• Laissez des commentaires sur la documentation de ce code, des commentaires et des messages de validation. De nombreux commentaires obstruent le code et signifient que les messages de validation déroutent ceux qui doivent ensuite travailler avec le code. Ce n'est pas toujours le cas, mais des commentaires de qualité et des messages de validation au fil du temps vous seront certainement utiles. (N'oubliez pas que vous avez vu un commentaire ou un message de validation magnifique ou tout simplement horrible.)
  
• La documentation externe a-t-elle été mise à jour? Si le LISEZMOI, le CHANGELOG ou toute autre documentation est conservé pour votre projet, est-il mis à jour pour refléter les modifications apportées? Une documentation obsolète peut être plus nocive que nulle, et il sera plus coûteux de corriger des erreurs à l'avenir que d'apporter des modifications maintenant.
  

N'oubliez pas de louer le code concis / lisible / efficace / beau. Au contraire, rejeter ou rejeter un code proposé pour révision n'est pas impoli. Si le changement est excessif ou insignifiant - rejetez-le en expliquant la raison. Si le changement vous semble inacceptable en raison d'une ou plusieurs erreurs fatales - rejetez-le, encore une fois, avec raison. Parfois, le meilleur résultat d'une révision de code est de dire «faisons-le complètement différemment» ou «ne le faisons pas du tout».

Respecté par les pairs. Bien que la position de l'adversaire soit saine, vous ne vous êtes pas rendu compte de cette opportunité et vous ne pouvez pas tout décider pour lui. Si vous ne pouvez pas obtenir une opinion évaluée par les pairs sur le code, organisez une consultation en temps réel et écoutez l'opinion d'une troisième personne.

La sécurité

Assurez-vous que tous les points de terminaison API sont correctement autorisés et authentifiés conformément aux règles adoptées dans le reste de la base de code. Recherchez d'autres failles courantes, par exemple: configuration faible, entrées d'utilisateurs malveillants, entrées de journal manquantes, etc. En cas de doute, montrez le code évalué par les pairs à un professionnel de la sécurité.

Commentaires: concis, poli, incitatif

L'examen doit être concis, soutenu dans un style neutre. Critiquez le code, pas l'auteur. Si quelque chose n'est pas clair - demandez des éclaircissements et ne présumez pas que le tout réside dans l'ignorance de l'auteur. Évitez les pronoms possessifs, en particulier dans les jugements de valeur: « mon code fonctionnait avant vos changements», «il y a un bug dans votre méthode», etc. Ne coupez pas l'épaule: «ça ne marchera pas du tout », «le résultat est toujours incorrect».

Essayez de faire la distinction entre les recommandations (par exemple, «Recommandation: extraire la méthode, cela rendra le code plus clair»), les modifications nécessaires (par exemple, «Ajouter un remplacement »), et les opinions qui nécessitent une clarification ou une discussion (par exemple, «ce comportement est-il vrai? ? Si oui, veuillez ajouter un commentaire expliquant la logique. ») Essayez de mettre des liens ou des pointeurs vers une description détaillée du problème.

Lorsque vous avez terminé la révision du code, indiquez dans quelle mesure la réaction à vos commentaires que vous attendez de l'auteur est détaillée, souhaitez-vous répéter la révision une fois les modifications apportées, par exemple: «Vous pouvez télécharger le code en toute sécurité dans la branche principale après ces corrections mineures» ou «Veuillez noter mes commentaires et faites-moi savoir quand je pourrai revoir le code. »

Examiner la réponse

La révision du code, en particulier, vise à améliorer la demande de modifications proposée par l'auteur; par conséquent, ne prenez pas l'hostilité des recommandations de l'examinateur, prenez-les au sérieux, même si vous n'êtes pas d'accord avec elles. Répondez à tout commentaire, même à l'habituel «ACK» (approuvé) ou «terminé» (terminé). Expliquez pourquoi vous avez pris telle ou telle décision, pourquoi vous avez certaines fonctions dans votre code, etc. Si vous ne parvenez pas à un avis commun avec le réviseur, organisez une consultation en temps réel et écoutez l'avis d'un tiers spécialiste.

Les corrections doivent être corrigées dans la même branche, mais dans un commit séparé. Si vous respectez les validations au stade de la révision, il sera plus difficile pour le réviseur de suivre les modifications.

La politique de fusion de code est différente pour différentes équipes. Dans certaines entreprises, la fusion de code n'est approuvée que par le propriétaire du projet; dans d'autres, un participant peut le faire après que son code a été examiné et approuvé.

Examen du code tête-à-tête

En règle générale, les outils de type diff asynchrones, tels que Reviewable, Gerrit ou GitHub, sont parfaits pour les révisions de code. Si nous parlons d'un examen complexe ou d'un examen, dont les participants diffèrent considérablement en termes d'expérience ou de professionnalisme, il peut être plus efficace d'effectuer un examen personnellement - soit assis ensemble devant un moniteur ou un projecteur, ou à distance, via des outils de vidéoconférence ou de partage d'écran.

Des exemples

Dans les exemples suivants, les commentaires des réviseurs dans les listes sont saisis à l'aide de

 //R: ... 

Dénomination incohérente

 class MyClass { private int countTotalPageVisits; //R:    private int uniqueUsersCount; } 

Signatures de méthode incohérentes

 interface MyInterface { /**  {@link Optional#empty},  s   . */ public Optional<String> extractString(String s); /**  null,  {@code s}   . */ //R:    :    // Optional<> public String rewriteString(String s); } 

Utilisation de la bibliothèque

 //R:     MapJoiner   Guava String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator); 

Préférences personnelles

 int dayCount; //R: :   numFoo   fooCount;   ,          

Bugs

 //R:    numIterations+1 –    ? //    –     numIterations? for (int i = 0; i <= numIterations; ++i) { ... } 

Considérations architecturales

 otherService.call(); //R: ,      OtherService.    ? 

À propos du traducteur

L'article a été traduit par Alconost.

Alconost localise des jeux , des applications et des sites dans 70 langues. Traducteurs en langue maternelle, tests linguistiques, plateforme cloud avec API, localisation continue, chefs de projet 24/7, tout format de ressources de chaîne.

Nous réalisons également des vidéos de publicité et de formation - pour les sites qui vendent, présentent des images, de la publicité, des formations, des teasers, des explicateurs, des bandes-annonces pour Google Play et l'App Store.

Plus de détails