Les révisions de code font partie intégrante de la création de logiciels robustes et de qualité. Chez Eagle Eye, elles constituent une partie importante du cycle de vie du développement logiciel, aidant nos ingénieurs à s'assurer qu'ils publient un meilleur code, et les aidant également à apprendre et à se développer. Ce qui suit est un ensemble de principes et de lignes directrices que nous encourageons nos ingénieurs à suivre chaque fois qu'ils se retrouvent à réviser le code de quelqu'un, ou que quelqu'un révise le leur.
Les revues de code sont une collaboration entre l'auteur du code et le réviseur, et la clé est de se faire confiance mutuellement. Tous deux sont animés des meilleures intentions, à savoir construire un logiciel qui résout un problème. Les revues de code ne sont pas là pour juger ou réprimander les gens, ou pour montrer à quel point quelqu'un est intelligent. Les critiques ne doivent pas être prises personnellement et doivent être aussi constructives que possible. Les revues de code sont des occasions d'apprendre les uns des autres et d'améliorer la qualité. Traiter les autres avec respect.
L'évaluateur donne de son temps pour aider à assurer la qualité de ce qui a été fait, et non pour corriger le code de l'auteur. L'auteur doit faire tout ce qui est en son pouvoir pour s'assurer qu'un code review est vraiment prêt à être revu en faisant quelques choses simples.
Une bonne revue de code commence avant même qu'une ligne de code ne soit écrite.
L'auteur doit s'assurer qu'il sait ce qu'il s'apprête à construire, et réfléchir soigneusement à la manière dont il compte l'aborder. Comprenez d'abord et avant tout les exigences de ce que vous devez faire, et clarifiez tout ce que vous ne comprenez pas avant même de penser à une solution potentielle.
Une fois que vous avez compris vos besoins et pensé à une solution, il est toujours bon de réunir deux ou trois autres ingénieurs pour en discuter au préalable, de préférence avec celui qui est susceptible d'examiner votre travail. Toutes les solutions ne sont pas suffisamment complexes ou importantes pour justifier une discussion approfondie, mais le fait de demander à quelqu'un d'y jeter un coup d'œil avant de la coder peut aider à mettre le doigt sur d'éventuelles faiblesses.
Vous serez surpris de voir la différence que cela peut faire en réduisant le temps nécessaire pour faire passer un projet en revue. L'identification précoce de l'examinateur du code est importante : l'examinateur sait ce qui l'attend et peut prendre le temps de comprendre le travail qu'il va examiner.
N'oubliez pas non plus d'allouer du temps aux revues de code lorsque vous estimez la durée de votre travail. Le manque de temps est l'un des principaux facteurs de la médiocrité des révisions de code. Il est utile d'en tenir compte dès le début.
Une bonne planification et une bonne préparation avant un voyage sont primordiales pour vous aider à atteindre votre destination.
Plus l'auteur crée ou modifie de code, plus la revue de code sera longue. L'auteur a l'avantage d'apprendre au fur et à mesure qu'il construit, alors qu'un réviseur de code ne voit que le produit fini. C'est pourquoi l'examen de petits morceaux de code est souvent plus rapide et plus sûr. Essayez de cibler au maximum l'étendue des modifications.
En supposant que vous utilisiez GitHub, l'auteur devrait donner à sa demande un bon titre qui décrit succinctement ce qui a été fait. Typiquement, je donne à mes demandes un titre au format `Ticket : Subject`. Comme nous utilisons Jira, nous utilisons le ticket Jira, mais si j'utilise les issues GitHub, j'utiliserai " Issue 123 ".
En termes de style, respectez le même style qu'un message de commit git : 50 caractères si possible, utilisez l'impératif et ne mettez pas de ponctuation à la fin de la phrase. Une fois qu'il aura été fusionné, votre journal git vous remerciera.
Pour la description, décrivez ce que vous avez fait, et pourquoi, si nécessaire. Utilisez des puces pour faciliter la lecture et la compréhension. Créez un lien vers toute demande de téléchargement connexe afin de signaler les dépendances. Soyez aussi simple et concis que possible.
Une bonne ligne d'objet et une bonne description aideront à mettre en évidence les domaines potentiellement importants sur lesquels l'examinateur du code doit se concentrer. Plus important encore, cela démontre la compréhension de l'auteur de ce qu'il a fait : si l'auteur ne peut pas décrire ce qui a été fait clairement et simplement, cela peut être un signe d'avertissement qu'il n'a peut-être pas complètement compris le ticket, ou le code lui-même.
Avant que l'auteur n'assigne la revue du code à quelqu'un, il devrait le revoir lui-même. Vous pouvez aussi utiliser `git diff` en ligne de commande, ou utiliser votre IDE pour voir les changements sur chaque fichier que vous avez modifié si vous souhaitez vérifier les changements avant même d'ouvrir votre pull request.
Il devient rapidement évident lors d'une revue de code que quelqu'un a ou n'a pas revu son propre travail, et cela peut être exceptionnellement frustrant.
Par analogie, imaginez que vous êtes un éditeur et qu'un auteur vous a envoyé un manuscrit à lire. Vous commencez à lire, et il est évident que l'auteur n'a pas édité son propre travail : chaque ligne est truffée de fautes d'orthographe et de grammaire. L'histoire est perdue dans un fouillis confus de répétitions et de trous dans l'intrigue. Il est peu probable que l'éditeur veuille perdre son temps.
De même, un examinateur de code n'a pas envie de perdre son temps sur des choses si évidentes que l'auteur aurait dû les repérer au premier coup d'œil. C'est un manque de respect et un moyen infaillible d'ennuyer vos collègues.
Une fois que l'auteur vous a remis la demande d'extraction, il est temps de commencer. Sauf qu'il ne faut pas commencer par la demande d'extraction. La demande d'extraction n'est qu'une partie de l'examen du code.
En tant que réviseur, vous devez d'abord savoir ce que vous révisez réellement. Vous devez lire et comprendre les exigences relatives au travail effectué : tickets connexes, histoires d'utilisateurs, exigences commerciales, spécifications. Comment pouvez-vous dire que quelque chose fonctionne si vous ne savez pas ce qui devait être construit ? Ce serait comme lire le CV d'une personne et l'approuver pour un emploi sans même savoir pour quel poste elle postule.
Cela peut sembler fastidieux, mais il est important de se rappeler que la grande majorité des logiciels échouent à cause des exigences, et pas nécessairement à cause du logiciel lui-même. Comprendre ce qui doit être construit permet de vérifier que les exigences sont valables. Si vous ne comprenez pas les exigences, demandez des éclaircissements, et si vous ne comprenez toujours pas, comment le développeur a-t-il pu coder quelque chose ? Quelque chose a-t-il été discuté et n'a-t-il pas été documenté ? Posez des questions au développeur et à d'autres personnes, si nécessaire, et demandez-leur de mettre à jour et de clarifier les documents, le cas échéant.
Une fois que vous avez compris ce qui doit être construit, et que vous commencez la revue de code, il peut être évident que les bases n'ont pas été faites. Si c'est le cas, rejetez la demande et renvoyez-la à l'auteur avec une brève note lui expliquant qu'il doit s'assurer qu'elle est prête.
Le fait est que le temps est précieux et qu'il n'appartient pas à un réviseur d'expliquer pourquoi les outils de CI ont échoué, de commenter les erreurs de formatage évidentes ou d'identifier le code qu'un IDE met en évidence. Ce genre de choses ne devrait pas nécessiter l'intervention d'un réviseur, et elles ajoutent beaucoup de bruit inutile, empêchant le réviseur de se concentrer sur le code lui-même.
Il est facile de ne penser qu'à ce qui se trouve devant soi, mais un examinateur doit prêter une attention particulière à ce qu'il ne voit pas.
Ils ont donc résolu le problème et le code fonctionne. Cependant, il est tout à fait possible qu'ils se soient complètement trompés de méthode. Cela peut être délicat à gérer, car il existe de nombreuses solutions différentes à un problème, mais il est parfois évident que le code peut être remanié d'une bien meilleure manière. Le repérage précoce de ce type de problème peut éviter bien des soucis par la suite.
Les choses simples doivent être simples, les choses complexes doivent être possibles. Abordez le code avec le point de vue que la perfection peut être atteinte parce qu'il n'y a rien à enlever. Le code le plus simple et le meilleur est le code qui n'est pas écrit. Le code peut-il être remanié en méthodes plus simples et réutilisables ? Peut-être savez-vous qu'un code similaire existe ailleurs dans la base de code, et que le moment est venu de le consolider en une source unique de vérité. Peut-être que de nouvelles classes peuvent être abstraites en quelque chose de beaucoup plus utile et réutilisable, et qu'elles partagent des points communs ?
Je crois fermement que l'optimisation prématurée est la racine de tous les maux, mais cela ne veut pas dire qu'il ne faut pas prêter attention à ce qui est évident. Les développeurs s'habituent souvent à disposer de grandes quantités de mémoire et de puissants processeurs multicœurs, mais ces éléments doivent toujours être pris en compte. Y a-t-il des problèmes potentiels de mémoire ou une utilisation excessive de l'unité centrale ? En outre, les requêtes de la base de données sont-elles optimales et utilisent-elles de bons index ? Y a-t-il des requêtes à l'intérieur de boucles ?
"D'abord, ne pas nuire." Tout le code doit s'efforcer de rendre les systèmes plus sûrs et meilleurs : validation robuste si nécessaire, traitement gracieux des erreurs et des exceptions, garantie que les données sont nettoyées avant d'être stockées dans la base de données, et prévention du déversement de données sensibles à l'écran. Réfléchissez à la question de savoir si le code introduit une dette technique supplémentaire.
Pour un homme armé d'un marteau, tout ressemble à un clou. Quelles sont les technologies utilisées ? Sont-elles utilisées correctement ? Le code a-t-il utilisé des modèles de conception et, dans l'affirmative, s'agit-il de l'outil adéquat ? Si aucun modèle n'a été utilisé, aurait-il pu l'être ? Examinez également s'il existe des anti-modèles dans le code.
C'est beaucoup plus important qu'on ne le pense, et c'est difficile. Les noms transmettent une signification et une compréhension aux autres développeurs. Si les noms des variables, des classes et des méthodes prêtent à confusion ou sont trompeurs, cela entravera le développement et le débogage futurs.
Veillez à ce que les messages soient clairs et simples. Il s'agit notamment des messages relatifs aux exceptions, à la journalisation, aux réponses de l'API, etc. Prêtez également attention aux commentaires. Sont-ils précis, utiles et significatifs ? Répètent-elles simplement ce que le code dit déjà ? Et s'il y a des choses à faire, demandez si elles ont été soulevées en tant que tickets dans votre carnet de commandes, ou si elles ont été portées à l'attention d'autres personnes.
Par ailleurs, il faut savoir que, dans certains systèmes, le stockage des messages de journalisation coûte de l'argent. Il est inutile de stocker des milliers de lignes de messages s'ils ne sont pas utiles
Le code n'existe pas de manière isolée. Il fait partie d'un ensemble beaucoup plus vaste. En tant que tel, vous devez réfléchir à la manière dont ce code s'intègre dans le reste de la base de code. Chaque base de code a ses propres idiosyncrasies et normes. Ont-ils été cohérents dans leur approche par rapport au reste de la base de code ? Si ce n'est pas le cas, cherchez à savoir pourquoi. Certaines parties du code devraient-elles se trouver dans un autre paquet ? Utilise-t-il le code existant ?
Nous travaillons souvent sur des systèmes anciens et tentaculaires qui existaient bien avant notre époque. Il est toujours tentant pour quelqu'un d'apporter le strict minimum de modifications à un code, de peur de toucher et de casser quelque chose.
Cependant, la peur fait une bonne armure, mais une mauvaise lame. Le code peut et doit changer. Tout comme nous ne devons pas faire de mal, nous devons essayer de laisser le code un peu meilleur que nous l'avons trouvé.
Parfois, il y a des changements à faible risque que l'on peut faire et qui aident à améliorer la base de code : corriger des erreurs évidentes de formatage du code, déclarer les types manquants dans les Blocks de doc, remplacer les méthodes dépréciées par la bonne. D'autres fois, les changements sont tout simplement nécessaires, même s'ils sont difficiles à réaliser, parce que si ce n'est pas maintenant, ce sera quand ? Même si le fait d'interroger le développeur à ce sujet aboutit à un ticket pour un travail futur, c'est mieux que de l'ignorer.
Le revers de la médaille est également vrai : ont-ils été trop bons en tant qu'éclaireurs et ont-ils corrigé des choses très risquées, qui ne sont pas liées à leur travail actuel et pour lesquelles il n'y a pas de tests ? Il doit y avoir un bon équilibre entre les changements apportés.
Il m'est souvent arrivé d'examiner un code et de penser que quelque chose était incorrect. Cependant, après avoir abordé l'auteur et discuté avec lui, je me suis rendu compte qu'il avait réfléchi à la question et qu'il avait tenu compte d'un élément qui m'avait échappé. Cela signifie parfois que je lui demande d'ajouter un commentaire au code pour le clarifier, mais le fait est qu'il vaut mieux demander à l'auteur du code pourquoi il a fait quelque chose d'une certaine manière, plutôt que de lui dire qu'il s'est trompé.
En outre, le fait d'avoir une conversation avec l'auteur peut démontrer qu'il a réfléchi à ce qu'il fait. Une explication courante que j'ai déjà entendue est la suivante : "J'ai fait comme ça, parce que c'était comme ça ailleurs". Pourtant, ils n'étaient pas en mesure d'expliquer ce qu'ils avaient réellement reproduit.
Cela démontre un manque de compréhension et constitue un bon signal d'alarme indiquant que le code a peut-être besoin d'un examen plus approfondi.
Cela nous ramène à l'idée qu'il ne s'agit pas seulement du code. La seule chose qui soit pire que l'absence de documentation, c'est une documentation incorrecte.
L'approbation du code ne doit pas être considérée comme une guerre d'usure. Le réviseur a son propre travail à faire, et l'auteur doit se concentrer sur d'autres tâches. Au fur et à mesure que les revues de code s'éternisent, il devient tentant d'y consacrer de moins en moins de temps, de se précipiter et de rater des choses, et de se contenter d'accepter ce qui a été fait en se disant : "Nous corrigerons cela plus tard". À moins d'être exceptionnellement discipliné, vous n'y parviendrez probablement jamais.
Afin de réduire les allers-retours d'une revue de code, il est important de faire ce qui suit :
Toutes les revues de code ne sont pas égales. Parfois, elles sont si petites et si simples que la plupart des éléments que j'ai mentionnés ici ne s'appliquent pas. L'important est de bien réfléchir à ce que vous faites, que vous soyez l'auteur ou l'évaluateur, et de communiquer clairement à toutes les étapes du processus de développement.
Responsable de l'ingénierie logicielle chez Eagle Eye
6 lecture des minutes
Miles Toolin on 15 juillet, 2024
Si vous prenez la peine de lire cet article, c'est que vous avez certainement déjà cherché sur Google ou ChatGPT : "Qu'est-ce qu'une architecture...
3 lecture des minutes
Rich Millington on 9 mai, 2024
La création d'un héritage : Les débuts Lorsque la société pour laquelle je travaillais à Manchester (2ergo) a été rachetée par Eagle Eye il y a un...
6 lecture des minutes
Tim Mason on 11 avril, 2024
Bonjour à tous et bienvenue à notre troisième article sur la Règle d'or ! Que vous soyez un nouveau venu ou que vous ayez besoin d'un petit rappel,...
