Bonnes pratiques de revue de code

.

L’Internet fournit une abondance de matériel sur les revues de code : sur l’effet des revues de code sur la culture d’entreprise, sur les revues de sécurité formelles, les guides plus courts, les listes de contrôle plus longues, les revues humanisées, les raisons de faire des revues de code en premier lieu, les meilleures pratiques, plus de meilleures pratiques, les statistiques sur l’efficacité des revues de code pour attraper les bogues, et des exemples de revues de code qui ont mal tourné. Oh, et bien sûr, il y a aussi des livres. Pour faire court, ce billet de blog présente le point de vue de Palantir sur les revues de code. Les organisations ayant une profonde réticence culturelle à l’égard des revues par les pairs voudront peut-être consulter l’excellent essai de Karl E. Wiegers sur l’humanisation des revues par les pairs avant d’essayer de suivre ce guide.

Ce billet est copié des guides de bonnes pratiques de notre chaîne d’outils de qualité du code Java, Baseline, et couvre les sujets suivants :

• Pourquoi, quoi et quand faire des revues de code
• Préparation du code pour la revue
• Exécution des revues de code
• Exemples de revues de code

Nous effectuons des revues de code (RC) afin d’améliorer la qualité du code et de bénéficier d’effets positifs sur la culture de l’équipe et de l’entreprise. Par exemple :

• Les committers sont motivés par la notion d’un ensemble de réviseurs qui se pencheront sur la demande de changement : le committer a tendance à faire le ménage, à consolider les TODO et à améliorer généralement le commit. La reconnaissance de l’expertise de codage par les pairs est une source de fierté pour de nombreux programmeurs.
• Le partage des connaissances aide les équipes de développement de plusieurs façons :
  – Un CR communique explicitement les fonctionnalités ajoutées/modifiées/supprimées aux membres de l’équipe qui peuvent ensuite s’appuyer sur le travail effectué.
  – Le committer peut utiliser une technique ou un algorithme dont les réviseurs peuvent apprendre. Plus généralement, les revues de code aident à élever la barre de qualité dans toute l’organisation.
  – Les réviseurs peuvent posséder des connaissances sur les techniques de programmation ou la base de code qui peuvent aider à améliorer ou à consolider le changement ; par exemple, quelqu’un d’autre peut travailler simultanément sur une fonctionnalité ou un correctif similaire.
  – Une interaction et une communication positives renforcent les liens sociaux entre les membres de l’équipe.
• La cohérence d’une base de code rend le code plus facile à lire et à comprendre, aide à prévenir les bogues et facilite la collaboration entre les espèces de développeurs réguliers et migrateurs.
• La lisibilité des fragments de code est difficile à juger pour l’auteur dont c’est le cerveau, et facile à juger pour un examinateur qui n’a pas le contexte complet. Un code lisible est plus réutilisable, exempt de bogues et à l’épreuve du futur.
• Les erreurs accidentelles (par exemple, les coquilles) ainsi que les erreurs structurelles (par exemple, le code mort, les bogues de logique ou d’algorithme, les problèmes de performance ou d’architecture) sont souvent beaucoup plus faciles à repérer pour les examinateurs critiques ayant une perspective extérieure. Des études ont montré que même les revues de code courtes et informelles ont un impact significatif sur la qualité du code et la fréquence des bogues.
• Les environnements de conformité et de réglementation exigent souvent des revues. Les CR sont un excellent moyen d’éviter les pièges de sécurité courants. Si votre fonctionnalité ou votre environnement a des exigences de sécurité importantes, il bénéficiera (et nécessitera probablement) d’une revue par vos curmudgeons de sécurité locaux (le guide de l’OWASP est un bon exemple du processus).

Que réviser

Il n’y a pas de réponse éternellement vraie à cette question et chaque équipe de développement devrait convenir de sa propre approche. Certaines équipes préfèrent revoir chaque changement fusionné dans la branche principale, tandis que d’autres auront un seuil de « trivialité » en dessous duquel une revue n’est pas nécessaire. Le compromis à trouver est celui de l’utilisation efficace du temps des ingénieurs (auteurs et réviseurs) et du maintien de la qualité du code. Dans certains environnements réglementaires, la revue de code peut être requise même pour des changements triviaux.

Les revues de code sont sans classe : être la personne la plus expérimentée de l’équipe n’implique pas que votre code n’a pas besoin d’être revu. Même si, dans le rare cas, le code est sans défaut, la revue offre une opportunité de mentorat et de collaboration, et, minimalement, diversifie la compréhension du code dans la base de code.

Quand réviser

Les revues de code devraient avoir lieu après que les vérifications automatisées (tests, style, autre CI) se soient terminées avec succès, mais avant que le code ne fusionne vers la branche principale du référentiel. Nous n’effectuons généralement pas de revue de code formelle des changements agrégés depuis la dernière version.

Pour les changements complexes qui devraient fusionner dans la branche principale en tant qu’unité unique mais qui sont trop grands pour tenir dans un CR raisonnable, envisagez un modèle de CR empilé : Créez une branche principale feature/big-feature et un certain nombre de branches secondaires (par exemple, feature/big-feature-apifeature/big-feature-testing, etc.) qui encapsulent chacune un sous-ensemble de la fonctionnalité et qui font l’objet d’une revue de code individuelle par rapport à la branche feature/big-feature. Une fois que toutes les branches secondaires sont fusionnées dans feature/big-feature, créez une CR pour fusionner cette dernière dans la branche principale.

Préparation du code pour la révision

Il incombe à l’auteur de soumettre des CR faciles à réviser afin de ne pas gaspiller le temps et la motivation des réviseurs :

• Portée et taille. Les changements devraient avoir une portée étroite, bien définie et autonome qu’ils couvrent de manière exhaustive. Par exemple, un changement peut implémenter une nouvelle fonctionnalité ou corriger un bug. Les changements courts sont préférés aux changements longs. Si une CR apporte des changements substantiels à plus de ~5 fichiers, ou si sa rédaction a pris plus de 1 à 2 jours, ou si sa révision prendrait plus de 20 minutes, envisagez de la diviser en plusieurs CR autonomes. Par exemple, un développeur peut soumettre un changement qui définit l’API d’une nouvelle fonctionnalité en termes d’interfaces et de documentation, et un second changement qui ajoute des implémentations pour ces interfaces.
• Ne soumettez que des CR complets, auto-révisés (par diff) et auto-testés. Afin d’économiser le temps des réviseurs, testez les changements soumis (c’est-à-dire, exécutez la suite de tests) et assurez-vous qu’ils passent tous les builds ainsi que tous les tests et contrôles de qualité du code, à la fois localement et sur les serveurs CI, avant d’assigner des réviseurs.
• Les changements de refactoring ne devraient pas modifier le comportement ; inversement, un changement de comportement devrait éviter le refactoring et les changements de formatage du code. Il y a de multiples bonnes raisons pour cela :
  – Les changements de refactoring touchent souvent de nombreuses lignes et fichiers et seront par conséquent revus avec moins d’attention. Des changements de comportement involontaires peuvent s’infiltrer dans la base de code sans que personne ne s’en aperçoive.
  – Les grands changements de refactoring cassent le cherry-picking, le rebasing, et d’autres magies de contrôle de source. Il est très onéreux d’annuler un changement de comportement qui a été introduit dans le cadre d’un commit de refactoring à l’échelle du dépôt.
  – Le temps coûteux de révision humaine devrait être consacré à la logique du programme plutôt qu’aux débats sur le style, la syntaxe ou le formatage. Nous préférons régler ceux-ci avec des outils automatisés comme Checkstyle, TSLint, Baseline, Prettier, etc.

Messages de commit

Voici un exemple de bon message de commit suivant une norme largement citée:

Capitalized, short (80 chars or less) summaryMore detailed explanatory text, if necessary. Wrap it to about 120 characters or so. In some contexts, the first
line is treated as the subject of an email and the rest of the text as the body. The blank line separating the
summary from the body is critical (unless you omit the body entirely); tools like rebase can get confused if you run
the two together.Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches
up with commit messages generated by commands like git merge and git revert.Further paragraphs come after blank lines.- Bullet points are okay, too

Essayez de décrire à la fois ce que le commit change et comment il le fait :

> BAD. Don't do this.
Make compile again> Good.
Add jcsv dependency to fix IntelliJ compilation

Trouver des réviseurs

Il est habituel que le committeur propose un ou deux réviseurs qui connaissent bien la base de code. Souvent, l’un des réviseurs est le chef de projet ou un ingénieur senior. Les propriétaires de projets devraient envisager de s’abonner à leurs projets afin d’être informés des nouveaux CR. Les revues de code entre plus de trois parties sont souvent improductives, voire contre-productives, car les différents réviseurs peuvent proposer des changements contradictoires. Cela peut indiquer un désaccord fondamental sur l’implémentation correcte et devrait être résolu en dehors d’une revue de code dans un forum à bande passante plus élevée, par exemple en personne ou en vidéoconférence avec toutes les parties impliquées.

Exécution des revues de code

Une revue de code est un point de synchronisation entre différents membres de l’équipe et a donc le potentiel de bloquer les progrès. Par conséquent, les revues de code doivent être rapides (de l’ordre de quelques heures, et non de quelques jours), et les membres de l’équipe et les responsables doivent être conscients de l’engagement de temps et prioriser le temps de revue en conséquence. Si vous ne pensez pas pouvoir terminer une revue à temps, faites-le savoir immédiatement au committer pour qu’il puisse trouver quelqu’un d’autre.

Une revue doit être suffisamment approfondie pour que le réviseur puisse expliquer la modification à un niveau de détail raisonnable à un autre développeur. Cela permet de s’assurer que les détails de la base de code sont connus de plus d’une seule personne.

En tant que réviseur, il est de votre responsabilité de faire respecter les normes de codage et de maintenir la barre de qualité vers le haut. La révision du code est plus un art qu’une science. La seule façon de l’apprendre est de le faire ; un réviseur expérimenté devrait envisager de mettre d’autres réviseurs moins expérimentés sur leurs modifications et leur demander de faire une révision en premier. En supposant que l’auteur a suivi les directives ci-dessus (en particulier en ce qui concerne l’auto-révision et l’assurance que le code s’exécute), voici une liste de choses auxquelles un réviseur devrait prêter attention dans une revue de code :

But

• Ce code accomplit-il le but de l’auteur ? Chaque changement devrait avoir une raison spécifique (nouvelle fonctionnalité, refactor, bugfix, etc). Le code soumis accomplit-il réellement cet objectif ?
• Prenez des questions. Les fonctions et les classes devraient exister pour une raison. Lorsque la raison n’est pas claire pour l’examinateur, cela peut être une indication que le code doit être réécrit ou soutenu par des commentaires ou des tests.

Mise en œuvre

• Réfléchissez à la façon dont vous auriez résolu le problème. Si c’est différent, pourquoi ? Votre code gère-t-il plus de cas (limites) ? Est-il plus court/facile/nettoyant/rapide/sûr tout en étant fonctionnellement équivalent ? Y a-t-il un modèle sous-jacent que vous avez repéré et qui n’est pas capturé par le code actuel ?
• Voyez-vous un potentiel pour des abstractions utiles ? Un code partiellement dupliqué indique souvent qu’un morceau de fonctionnalité plus abstrait ou général peut être extrait puis réutilisé dans différents contextes.
• Pensez comme un adversaire, mais soyez gentil. Essayez d' » attraper  » les auteurs qui prennent des raccourcis ou qui manquent des cas en venant avec des configurations/données d’entrée problématiques qui cassent leur code.
• Pensez aux bibliothèques ou au code produit existant. Lorsque quelqu’un réimplémente une fonctionnalité existante, le plus souvent, c’est simplement parce qu’il ne sait pas qu’elle existe déjà. Parfois, le code ou la fonctionnalité est dupliqué à dessein, par exemple pour éviter les dépendances. Dans ce cas, un commentaire de code peut clarifier l’intention. La fonctionnalité introduite est-elle déjà fournie par une bibliothèque existante ?
• La modification suit-elle des modèles standard ? Les bases de code établies présentent souvent des modèles autour des conventions de nommage, de la décomposition de la logique du programme, des définitions des types de données, etc. Il est généralement souhaitable que les changements soient mis en œuvre conformément aux modèles existants.
• Le changement ajoute-t-il des dépendances de compilation ou d’exécution (en particulier entre les sous-projets) ? Nous voulons garder nos produits faiblement couplés, avec aussi peu de dépendances que possible. Les modifications apportées aux dépendances et au système de construction doivent être examinées de près.

Légibilité et style

• Pensez à votre expérience de lecture. Avez-vous saisi les concepts dans un laps de temps raisonnable ? Le flux était-il sain et les noms des variables et des méthodes étaient-ils faciles à suivre ? Avez-vous été en mesure de garder la trace à travers plusieurs fichiers ou fonctions ? Avez-vous été rebuté par un nommage incohérent ?
• Le code respecte-t-il les directives de codage et le style de code ? Le code est-il cohérent avec le projet en termes de style, de conventions API, etc. Comme mentionné ci-dessus, nous préférons régler les débats de style avec des outils automatisés.
• Ce code comporte-t-il des TODO ? Les TODOs ne font que s’empiler dans le code, et deviennent périmés avec le temps. Demandez à l’auteur de soumettre un ticket sur GitHub Issues ou JIRA et attachez le numéro de l’issue au TODO. Le changement de code proposé ne doit pas contenir de code commenté.

Maintenabilité

• Lisez les tests. S’il n’y a pas de tests et qu’il devrait y en avoir, demandez à l’auteur d’en écrire. Les fonctionnalités réellement non testables sont rares, tandis que les implémentations non testées de fonctionnalités sont malheureusement courantes. Vérifiez les tests eux-mêmes : couvrent-ils des cas intéressants ? Sont-ils lisibles ? La RC réduit-elle la couverture de test globale ? Pensez aux façons dont ce code pourrait se casser. Les normes de style pour les tests sont souvent différentes de celles du code de base, mais elles restent importantes.
• Cette RC introduit-elle le risque de casser le code de test, les staging stacks ou les tests d’intégration ? Ceux-ci ne sont souvent pas vérifiés dans le cadre des vérifications de pré-commit/merge, mais les faire tomber est douloureux pour tout le monde. Les choses spécifiques à rechercher sont : la suppression des utilitaires ou des modes de test, les changements de configuration et les changements dans la disposition/structure des artefacts.
• Cette modification rompt-elle la compatibilité ascendante ? Si c’est le cas, est-il acceptable de fusionner la modification à ce stade ou doit-elle être poussée dans une version ultérieure ? Les ruptures peuvent inclure des changements de base de données ou de schéma, des changements d’API publique, des changements de flux de travail des utilisateurs, etc.
• Ce code nécessite-t-il des tests d’intégration ? Parfois, le code ne peut pas être testé de manière adéquate avec des tests unitaires seuls, en particulier si le code interagit avec des systèmes ou une configuration extérieurs.
• Laisser des commentaires sur la documentation, les commentaires et les messages de validation au niveau du code. Les commentaires redondants encombrent le code, et les messages de commit laconiques mystifient les futurs contributeurs. Ce n’est pas toujours applicable, mais des commentaires et des messages de commit de qualité se paieront d’eux-mêmes à la longue. (Pensez à une fois où vous avez vu un message commit ou un commentaire excellent, ou vraiment terrible.)
• La documentation externe a-t-elle été mise à jour ? Si votre projet maintient un README, un CHANGELOG, ou une autre documentation, a-t-elle été mise à jour pour refléter les changements ? Une documentation périmée peut être plus déroutante que l’absence de documentation, et il sera plus coûteux de la corriger à l’avenir que de la mettre à jour maintenant.

N’oubliez pas de faire l’éloge d’un code concis/lisible/efficace/élégant. À l’inverse, refuser ou désapprouver une RC n’est pas impoli. Si la modification est redondante ou non pertinente, déclinez-la avec une explication. Si vous la considérez comme inacceptable en raison d’une ou plusieurs failles fatales, désapprouvez-la, toujours avec une explication. Parfois, le bon résultat d’une RC est  » faisons-le d’une manière totalement différente  » ou même  » ne le faisons pas du tout. « 

Soyez respectueux envers les personnes révisées. Bien que la pensée contradictoire soit pratique, ce n’est pas votre fonction et vous ne pouvez pas prendre toutes les décisions. Si vous ne pouvez pas parvenir à un accord avec votre révisé avec le code tel quel, passez à une communication en temps réel ou demandez un troisième avis.

Sécurité

Vérifiez que les points de terminaison de l’API effectuent une autorisation et une authentification appropriées, cohérentes avec le reste de la base de code. Vérifiez les autres faiblesses courantes, par exemple, une configuration faible, une entrée utilisateur malveillante, des événements de journal manquants, etc. En cas de doute, renvoyez le CR à un expert en sécurité des applications.

Commentaires : concis, amicaux, exploitables

Les critiques doivent être concises et rédigées dans un langage neutre. Critiquez le code, pas l’auteur. Lorsque quelque chose n’est pas clair, demandez des éclaircissements plutôt que de supposer l’ignorance. Évitez les pronoms possessifs, en particulier en conjonction avec les évaluations : « mon code fonctionnait avant votre modification », « votre méthode a un bug », etc. Évitez les jugements absolus : « cela ne peut jamais fonctionner », « le résultat est toujours faux ».

Tentez de différencier les suggestions (par exemple, « Suggestion : extraire la méthode pour améliorer la lisibilité »), les changements requis (par exemple, « Ajouter @Override ») et les points qui nécessitent une discussion ou une clarification (par exemple, « Est-ce vraiment le bon comportement ? Si oui, veuillez ajouter un commentaire expliquant la logique. »). Envisagez de fournir des liens ou des pointeurs vers des explications approfondies d’un problème.

Quand vous avez terminé la révision du code, indiquez dans quelle mesure vous attendez de l’auteur qu’il réponde à vos commentaires et si vous souhaitez réexaminer le CR après la mise en œuvre des modifications (par ex,  » N’hésitez pas à fusionner après avoir répondu aux quelques suggestions mineures  » vs  » Veuillez prendre en compte mes suggestions et me faire savoir quand je pourrai y jeter un autre coup d’œil « ).

Répondre aux revues

Une partie de l’objectif de la revue de code est d’améliorer la demande de changement de l’auteur ; par conséquent, ne soyez pas offensé par les suggestions de votre réviseur et prenez-les au sérieux même si vous n’êtes pas d’accord. Répondez à chaque commentaire, même s’il ne s’agit que d’un simple « ACK » ou « done ». Expliquez pourquoi vous avez pris certaines décisions, pourquoi certaines fonctions existent, etc. Si vous ne parvenez pas à vous mettre d’accord avec le réviseur, passez à une communication en temps réel ou demandez un avis extérieur.

Les correctifs doivent être poussés vers la même branche, mais dans un commit distinct. Écraser les commits pendant le processus de révision rend difficile le suivi des changements par le réviseur.

Des équipes différentes ont des politiques de fusion différentes : certaines équipes autorisent uniquement les propriétaires de projet à fusionner, tandis que d’autres permettent au contributeur de fusionner après une révision de code positive.

Revues de code en personne

Pour la majorité des revues de code, les outils asynchrones basés sur le diff tels que Reviewable, Gerrit ou, GitHub sont un excellent choix. Les modifications complexes, ou les revues entre des parties ayant une expertise ou une expérience très différente peuvent être plus efficaces lorsqu’elles sont effectuées en personne, soit devant le même écran ou projecteur, soit à distance via des outils de VTC ou de partage d’écran.

Exemples

Dans les exemples suivants, les commentaires de revue suggérés sont indiqués par des //R: ... commentaires dans les blocs de code.

Nommage incohérent

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

Signatures de méthodes incohérentes

interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<String> extractString(String s); /** Returns null if {@code s} cannot be rewritten. */
//R: should harmonize return values: use Optional<> here, too
public String rewriteString(String s);
}

Utilisation de la bibliothèque

//R: remove and replace by Guava's MapJoiner
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

Goût personnel

int dayCount; //R: nit: I usually prefer numFoo over fooCount; up to you, but we should keep it consistent in this project

Bugs

//R: This performs numIterations+1 iterations, is that intentional?
// If it is, consider changing the numIterations semantics?
for (int i = 0; i <= numIterations; ++i) {
...
}

Problèmes d’architecture

otherService.call(); //R: I think we should avoid the dependency on OtherService. Can we discuss this in person?