Culture de révision de code : pourquoi vous devez en avoir une

Combien de fois croisez-vous les doigts lorsque vous ouvrez une pull request dans l'espoir qu'elle ne soit pas attribuée à ce développeur de votre équipe qui laisse toujours une poignée de commentaires sur tout et n'importe quoi ? Je ne parle pas de ceux très raisonnables qui révèlent des erreurs commises ou, s'il vous plaît, non, des odeurs de code.

Il s'agit des commentaires sur votre style de code ou de quelques petites choses subjectives qui ne visent pas à améliorer la base de code au final, mais qui donnent plutôt l'impression que « j'aurais fait les choses d'une autre manière parce que je les aime plus ». En tant que coéquipier responsable, vous êtes susceptible de les aborder.

Quelle que soit l'option que vous choisirez (appliquer simplement tous les changements ou refuser en expliquant longuement pourquoi vous n'êtes pas d'accord sur tel ou tel point), vous finirez probablement par vous sentir agité, épuisé et frustré par le temps passé sur des choses non essentielles. Et est-ce que cela en valait vraiment la peine ? Cette question vous viendra à l'esprit chaque fois que vous serez confronté à de telles situations.

Parfois, vous avez l'autre bout du bâton : Instagram approuve sans aucun commentaire et sans aucun signe qu'un examinateur a réellement vérifié soigneusement vos modifications. Qu'il l'ait examiné et qu'il n'y ait pas d'apports objectifs ou non, vous vous posez des questions sur vous-même et sur ce coéquipier.

Certes, même si tout va bien, ne serait-il pas agréable d'inclure un rapide message à l'auteur et de vous assurer que vous êtes sur la même longueur d'onde sans aucun doute ?

Si les deux histoires ci-dessus vous semblent trop familières, cet article est pour vous. Ce qui manque à votre équipe, c'est une culture des pull requests ou, en d'autres termes, un ensemble de lignes directrices sur la façon de communiquer dans les pull requests pour soutenir un processus de collaboration convivial et hautement efficace. Je couvrirai les éléments essentiels que les réviseurs voudront rechercher ainsi que des exemples de commentaires douteux - plus simples, des pinaillages - qui créent des frictions ou parfois même des problèmes au-delà de vos sentiments personnels.

En tant que développeur iOS moi-même, j'utiliserai Swift dans mes exemples de code, mais en général, ce sujet est important pour tout développeur, quelle que soit la plate-forme ou le langage que vous utilisez au quotidien.

Cependant, je pense que la culture de la révision de code est encore plus pertinente pour les plateformes Apple car, d'après mon expérience, nous avons tendance à être plus exigeants à bien des égards. Cela vient probablement d'un état d'esprit perfectionniste hérité de la vision de Steve à l'époque.

Leçon apprise

Une demande de tirage ou de fusion est un moyen de vérifier les modifications apportées au code avec vos collègues et de vérifier s'il contient des erreurs, d'évaluer son état de préparation avant de passer aux étapes suivantes et, enfin, de le communiquer aux utilisateurs finaux. C'est également l'un des principaux canaux de communication entre les développeurs. Parfois, nous ne connaissons même pas une personne en dehors des threads dans les PR.

Bien souvent, surtout au début de leur carrière, les développeurs ne prêtent pas attention à leur communication RP. En particulier, ils ne se soucient pas de la manière dont leurs commentaires peuvent être perçus par les autres, de la clarté et de la bonne rédaction des points soulevés en anglais, etc.

Croyez-moi, j'ai vécu cette expérience et j'ai moi-même commis des erreurs par le passé. Une expérience en particulier m'a marqué jusqu'à aujourd'hui. Je l'évoque presque à chaque fois lors des discussions sur la culture des relations publiques car elle met parfaitement en évidence l'importance de ce sujet.

Il y a quelques années, alors que je cherchais un nouvel emploi, j'ai été confronté à une demande d'extraction simulée que je devais examiner. Je n'ai pas réfléchi à tout à fond cette fois-ci parce que j'étais excité d'avoir reçu une tâche facile au lieu d'une autre torture Leetcode. Bien sûr, ce n'était pas si facile. C'était un test sur la culture des demandes d'extraction auquel j'ai échoué de manière drastique.

J'ai parcouru rapidement le communiqué de presse et j'ai laissé un tas de commentaires pour la plupart inutiles, par exemple :

 import UIKit import AVFoundation // It would be nice to list your imports alphabetically, so that it's easier to read.

Ou un autre :

 func someMethod() { // about 30 lines of code } // What do you think about splitting this method in half?

Dans d'autres rares commentaires où j'ai repéré un problème, je n'ai pas été assez clair à ce sujet et je n'ai pas fourni de solution raisonnable. Mais le principal échec de cet exercice a été que j'ai raté quelques problèmes de performance parce que je n'étais pas concentré au bon endroit. Au final, j'ai reçu le retour suivant :

J'ai senti que vous aviez manqué beaucoup de détails importants, comme l'exactitude du code, les performances et les améliorations générales (à quelques exceptions près).

Vous avez concentré une grande partie de la révision sur le formatage, qui devrait faire l'objet d'une discussion en équipe et/ou faire appel à des outils de formatage. Cela permet également aux développeurs de se concentrer sur les bons éléments lors d'une révision.

Depuis lors, j’ai réalisé que mon approche des revues de code était loin d’être parfaite et manquait de nombreux éléments importants.

Cet exemple est assez primitif d'une certaine manière, mais il montre clairement comment le manque d'une culture des relations publiques peut facilement devenir un chemin dangereux menant directement à des bugs et des problèmes en production.

Plongeons dans les détails et commençons par quelque chose que vous voudriez éviter en tant que réviseur de code : écrire des commentaires pointilleux.

Qu'est-ce qu'un commentaire pinailleur ?

Il existe de nombreuses façons d'exprimer votre logique et vos pensées dans le code. Les langages de programmation modernes vous offrent suffisamment de polyvalence pour le faire, et souvent, il n'y a pas de bonne ou de mauvaise approche entre plusieurs approches. Cependant, nous ne voulons pas que nos projets ressemblent à des monstres de Frankenstein : chaque équipe doit avoir un style de code décent avec des lignes directrices.

En même temps, nous ne pouvons pas et ne devons pas contrôler chaque ligne de code de nos coéquipiers. Je crois que trouver un bon équilibre entre les deux est une vertu que nous souhaitons idéalement atteindre.

Un commentaire de pinaillerie est une demande de modification, généralement minime, qui n'a pas de raison objective de le faire. Il s'agit souvent d'une projection d'une préférence personnelle d'un réviseur de code dans le code examiné. Laissez-moi vous en montrer quelques exemples :

 // Original guard let newValue, newValue != oldValue else { return } // Change request guard let newValue, newValue != oldValue else { return }

Si vous me demandez, je choisirais toujours la deuxième option car il est plus facile à mon avis de déboguer un tel code et de mettre un point d'arrêt directement sur la ligne de retour. Mais vous pouvez argumenter que la première variante économise 2 lignes, et vous pouvez également voir ce style chez les ingénieurs d'Apple.

 // Original func handleErrorIfNeeded(_ error: SomeError) { if error == .failedRequest { return } if error == .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases } // Change request func handleErrorIfNeeded(_ error: SomeError) { guard error != .failedRequest else { return } guard error != .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases }

Dans de nombreuses situations logiques, guard et if dans Swift peuvent être utilisés de manière interchangeable. Cependant, je dirais qu'ils n'ont pas la même sémantique de langage. guard , comme son nom l'indique, veut « protéger » votre flux de code d'un résultat indésirable, tandis que if est neutre par nature et permet une probabilité de 50 à 50 % que des chemins de code différents se produisent. Cependant, les deux variantes ne sont techniquement pas fausses et parfaitement lisibles.

Et le dernier :

 // Original, the constant value is not being repeated anywhere else func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(130) // … calculation return finalDuration } // Change request func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(Constant.startDuration) // … calculation return finalDuration } enum Constant { static let startDuration = Double(130) }

Vous pouvez argumenter sur la nécessité d'extraire toute constante dans un espace de noms séparé si elle n'est utilisée qu'une seule fois et lorsqu'il ne s'agit pas d'une constante magique. Dans la première variante, vous savez clairement quel est ce nombre de 130. Cependant, quelqu'un vous dira d'extraire n'importe quelle constante comme celle-ci, quoi qu'il en soit.

Tous les exemples ci-dessus sont des modifications mineures. Les deux variantes sont parfaitement correctes, lisibles par tout le monde et ne cassent pas votre code. Ce sont simplement des alternatives ayant le même poids en termes d'utilisation.

Alors, que voulez-vous faire avec des commentaires comme ceux-ci ? Du point de vue d'un critique, certains diront quelque chose comme ceci :

Eh bien, ce n'est qu'une suggestion. Il n'est pas obligatoire de l'appliquer.

C'est vrai, mais en même temps, il est important d'en informer l'auteur, par exemple :

Ce n'est qu'une suggestion, n'hésitez pas à la rejeter, mais que pensez-vous de cette approche : … ?

Cependant, mon avis sur les critiques est de ne pas les publier du tout . Pourquoi ?

Supposons que vous ayez soumis un commentaire insignifiant, en soulignant même qu'il s'agit d'une suggestion de faible priorité. Que se passe-t-il ensuite ?

• 1ère option : l'auteur applique le changement. Qu'est-ce que cela signifie réellement en coulisses ?
  • Ils doivent sauvegarder les modifications qu’ils sont en train d’effectuer.
  • Ils doivent ensuite passer à la branche PR.
  • Ils doivent appliquer le changement.
  • Ils pourraient résoudre les conflits.
  • Ils doivent impulser le changement.
  • Ensuite, cela déclenchera une autre série de tests CI en raison du nouveau code envoyé.
  • Ils doivent répondre au commentaire avec du texte ou un emoji.

D'un côté, c'est un ensemble très simple de tâches que nous effectuons tous les jours, en tant que développeurs. Mais imaginez, ce n'est pas un commentaire de pinaille, mais 10 à la place. Ou alors vous avez 5 pull requests ouvertes en même temps avec des pinailles de quantités différentes. Est-ce que cela valait vraiment la peine de perdre votre temps et de vous tracasser ? Quand on sait ce qu'est une pinaille, la réponse est non. Je préfère livrer mes modifications plus rapidement et gagner du temps.

• 2ème option : l'auteur n'applique pas la modification.

  • Dans de nombreuses situations, il est tout simplement impoli de ne pas tenir compte du commentaire de votre coéquipier sans même y répondre. Et cela peut également paraître impoli de simplement répondre avec :

    
    

    Merci pour la suggestion, mais je ne l'aime pas. Je ne l'appliquerai pas, car je ne pense pas que ce soit nécessaire.

  
  

  • Au lieu de cela, en tant qu'auteur, vous expliquerez probablement en détail pourquoi vous n'appliquerez pas les modifications. Par exemple, votre vision est en guard par rapport à celle let comme ci-dessus.

    
    

  • Il serait alors formidable que le critique accepte votre point de vue et passe à autre chose. Mais il pourrait aussi ressentir une certaine opposition et réagir parce qu'il pense sincèrement que sa méthode est meilleure.

    
    

  • De tels fils de discussion peuvent facilement prendre des proportions démesurées avec de longs commentaires qui vont et viennent, ce qui n'est pas du tout productif. Et cela ne mène à rien au final. Vous pouvez tous les deux convenir de ne pas être d'accord, il n'y a pas de « gagnant », mais la question finale est la même. Est-ce que cela valait vraiment la peine d'y consacrer du temps et des efforts ?

  
  

Ma pratique consiste à filtrer ces détails dans les revues de code en tant que réviseur avant même de publier quoi que ce soit en me posant ces questions :

Est-ce vraiment important de changer quelque chose ?

Y a-t-il des raisons objectives à cela ? Si oui, nommez-les.

Est-ce que je souhaite publier cette remarque simplement parce que j'aurais agi différemment ? Ou bien il y a une réelle inquiétude ?

De cette façon, vous pouvez supprimer le « bruit » des commentaires pointilleux de votre révision de code et ne laisser que les parties importantes, celles sur lesquelles vous devez vous concentrer sur l’évolutivité, la robustesse, la sécurité des threads, etc.

Mais pourquoi de tels commentaires apparaissent-ils ? En général, c'est le signe que les examinateurs n'ont pas une idée claire de ce sur quoi se concentrer ou de ce sur quoi chercher dans les demandes d'extraction. Dans la section suivante, nous aborderons exactement ce sujet.

Comment créer une culture de révision de code

Voyons comment nous pouvons créer un modèle de « référence absolue » en matière de relations publiques. Je vais énumérer un ensemble de valeurs et de lignes directrices qui sont essentielles à mon avis pour améliorer vos revues de code :

1. Pour tous : définissez un ensemble de compartiments ou de catégories pour vos commentaires de révision de code et attribuez-leur différents niveaux de gravité. Montrez toujours des exemples à vos collègues, afin que vous soyez tous sur la même longueur d'onde. Exemple :

…Et ainsi de suite. Cela peut être différent dans votre équipe, mais l’idée ici est d’avoir une structure et une classification. Mettre en place des valeurs de gravité nous permet de mieux hiérarchiser et de nous concentrer sur l’essentiel.

2. Réviseur : Essayez d'allouer un créneau horaire spécifique à vos révisions de code. Ne vous précipitez pas et permettez-vous d'approfondir la signification des modifications de code, pas la syntaxe. Cela devrait améliorer la qualité et la clarté de vos commentaires.

   
   

3. Auteur : Aidez vos pairs. Si vous savez que certaines parties peuvent être floues en raison du manque de contexte ou parce que votre PR n'est qu'une partie d'une série de choses à venir, laissez simplement un commentaire avec une explication sur votre propre pull request à l'avance. Cela fera gagner du temps aux deux parties !

   
   

4. Réviseur : relisez vos commentaires avant de les publier. Mettez-vous à la place de l'autre partie. Tout est-il clair ? Essayez de vous en tenir à la structure suivante :

   1. Exprimez votre préoccupation.
   2. Pourquoi c'est important.
   3. Que proposez-vous de faire à la place ?

   
   

5. Pour tous : vérifiez votre grammaire et votre orthographe en anglais. Oui, pour beaucoup d'entre nous, l'anglais n'est pas notre langue maternelle, et c'est parfois difficile à faire. Mais si ce n'est pas bien écrit, cela peut conduire à toutes sortes de confusions et de désalignements que vous ne souhaitez pas avoir.

   
   

6. Pour tous : Comment votre commentaire est-il perçu ? Est-il aimable ? Ce point est assez similaire au précédent, mais l'idée est de garder une approche saine et amicale dans la forme écrite. Par exemple :

   
   

   Vous devez extraire cette logique dans une méthode distincte.

   
   

   Même si cette phrase ne présente aucune anomalie grammaticale, le mot « must » est rarement utilisé dans ce contexte et peut être considéré comme un ordre donné au lecteur. Les représentants du gouvernement vous diront ce que vous devez faire parce qu'il existe des lois. Mais ce n'est pas le type de formulation que vous souhaitez utiliser avec vos collègues lorsque vous faites partie de la même équipe. Essayez plutôt ceci :

   
   

   Que pensez-vous de l’extraction de cette logique dans une méthode distincte ?

   
   

7. Critiqueur : Équilibrez le négatif et le positif. Si vous avez laissé quelques commentaires critiques, trouvez-en de bons et mettez-les également en valeur. C'est une petite chose, mais cela améliore la perception globale aux yeux de l'auteur.

   
   

8. Réviseur : si tout va bien dans les changements de code, n'hésitez pas à publier des éloges. Approuver sur Instagram sans laisser de signe n'est pas un bon comportement et laisse beaucoup de questions ouvertes. Publiez un message de remerciement avec les choses exactes que vous avez trouvées excellentes dans le travail de votre collègue. Un simple « LGTM » est trop générique et peut être perçu comme dédaigneux.

Je pense que ce sont de bons points à mettre en place au sein de votre équipe pour travailler à une communication meilleure et plus saine dans vos demandes d'extraction. Le plus difficile ensuite est de continuer à suivre ces directives. Parfois, cela peut être deux pas en avant et un pas en arrière.

Conclusion

Au final, le codage n’est pas toujours la partie la plus difficile. La communication et la collaboration peuvent être plus difficiles, surtout lorsque vous venez tous de pays et d’horizons différents, avec vos propres expériences et pensées.

J’espère que cet aperçu de la culture de la révision de code vous a été utile et que vous en tirerez peut-être quelque chose d’utile.

Bonne chance et bonnes revues de code à vous tous !