Comment rendre les révisions de code plus rapides et plus efficaces

Comment se déroule généralement une révision de code? Vous envoyez une demande de pool, obtenez des commentaires, apportez des corrections, envoyez des correctifs pour un deuxième examen, puis obtenez l'approbation et la fusion se produit. Cela semble simple, mais en réalité, le processus d'examen peut prendre beaucoup de temps.

Imaginez que vous ayez une demande de pool avec des centaines de lignes de changement. Le réviseur doit consacrer beaucoup de temps à lire entièrement le code et à comprendre les modifications proposées. Par conséquent, l'ensemble du processus, de la création d'une demande de pool à son approbation, peut prendre plusieurs jours - ce qui n'est pas très agréable pour le réviseur et l'auteur des modifications. Et les chances sont bonnes qu'en fin de compte, le critique manque encore quelque chose. Ou le contrôle peut être trop superficiel et, dans le pire des cas, la demande de pool peut être rejetée immédiatement.

Il s'avère que plus la demande de pool est grande, moins il sera avantageux de la vérifier.

Comment éviter de telles situations? Comment rendre la demande de pool plus facile et plus compréhensible pour le réviseur et optimiser l'ensemble du processus?

Nous traduisons un article de notre développeur principal Sergey Zhuk sur le fonctionnement du processus de révision du code de l'équipe de l'application mobile Skyeng.

Modifier les catégories

Imaginons que vous ayez une tâche: implémenter de nouvelles fonctionnalités dans le projet. La demande de pool sur laquelle vous travaillez peut contenir différentes catégories de modifications. Bien sûr, il y aura du nouveau code dedans. Mais au cours du travail, vous remarquerez peut-être que certains codes doivent être refactorisés au préalable afin de contribuer à l'ajout de nouvelles fonctionnalités. Ou avec cette nouvelle fonctionnalité, la duplication est apparue dans le code que vous souhaitez éliminer. Ou vous avez soudainement trouvé une erreur et souhaitez la corriger. À quoi devrait ressembler la demande de pool finale?

Voyons d'abord quelles catégories de modifications peuvent se produire avec le code.

1. Changements fonctionnels.
2. Refactoring structurel - changements de classes, interfaces, méthodes, mouvement entre classes.
3. Refactorisation simple - peut être effectuée à l'aide de l'EDI (par exemple, extraction de variables / méthodes / constantes, simplification des conditions).
4. Renommer et déplacer des classes - réorganiser l'espace de noms.
5. Suppression du code inutilisé (mort).
6. Corrections de style de code.

Examen de la stratégie

Il est très important de comprendre que chacune de ces catégories est examinée différemment et lorsque vous les examinez, l'examinateur doit se concentrer sur des choses différentes. On peut dire que chaque catégorie de changement implique sa propre stratégie d'examen.

1. Changement fonctionnel: résolution de problèmes commerciaux et conception de systèmes.
2. Refactoring structurel: rétrocompatibilité et amélioration de la conception.
3. Refactoring primitif: lisibilité améliorée. Ces modifications peuvent être principalement effectuées à l'aide de l'EDI (par exemple, extraire des variables / méthodes / constantes, etc.).
4. Renommer / déplacer des classes: la structure de l' espace de noms s'est-elle améliorée?
5. Suppression du code inutilisé: rétrocompatible.
6. Corrections de style de code: le plus souvent, la demande de fusion de pool se produit immédiatement.

Différentes approches sont utilisées pour vérifier les changements dans différentes catégories, par conséquent, le temps et les efforts consacrés à leur examen varient également.

Changements fonctionnels. Il s'agit du processus le plus long car il implique de modifier la logique du domaine. Le réviseur cherche à voir si le problème est résolu et vérifie si la solution proposée est la plus appropriée ou peut être améliorée.

Refactoring structurel. Ce processus nécessite beaucoup moins de temps que les changements fonctionnels. Mais il peut y avoir des suggestions et des désaccords sur l'organisation exacte du code.

Lors de la vérification des catégories restantes dans 99% des cas, la fusion se produit immédiatement.

1. Refactoring simple. Le code est-il devenu plus lisible? - merzhim.
2. Renommer / déplacer des classes. La classe a-t-elle été déplacée vers un meilleur espace de noms? - Merzh.
3. La suppression du code inutilisé (mort) est merzhim.
4. Corrections du style de code ou de la mise en forme - merzh. Vos collègues ne devraient pas vérifier cela pendant la révision du code, c'est la tâche du linter.

Pourquoi les changements devraient-ils être classés?

Nous avons déjà expliqué que différentes catégories de changements sont révisées différemment. Par exemple, nous vérifions les changements fonctionnels en fonction des besoins de l'entreprise, et dans le refactoring structurel, nous vérifions la compatibilité descendante. Et si nous mélangeons plusieurs catégories, il sera difficile pour le réviseur de garder à l'esprit plusieurs stratégies de révision en même temps. Et, très probablement, le réviseur passera plus de temps sur la demande de pool que nécessaire, et à cause de cela, il peut manquer quelque chose. De plus, si la demande de pool contient différents types de changements, avec toute correction, le réviseur devra réviser à nouveau toutes ces catégories. Par exemple, vous avez mélangé le refactoring structurel et les changements fonctionnels. Même si le refactoring est bien effectué, mais qu'il y a un problème avec la mise en œuvre de la fonctionnalité, après les corrections, le réviseur devra examiner la demande de pool entière dès le début. Autrement dit, revérifiez à la fois le refactoring et les modifications fonctionnelles. Le réviseur passe donc plus de temps sur la demande de pool. Au lieu d'envisager immédiatement une demande de pool séparée avec refactoring, le réviseur devrait à nouveau revoir le code entier.

Ce qui ne vaut pas la peine d'être mélangé

Renommer / supprimer une classe et sa refactorisation. Nous rencontrons ici Git, qui ne comprend pas toujours correctement ces changements. Je veux dire des changements à grande échelle lorsque de nombreuses lignes changent. Lorsque vous refactorisez une classe et que vous la déplacez quelque part, Git ne la perçoit pas comme se déplaçant. Au lieu de cela, Git interprète ces changements comme supprimant une classe et en créant une autre. Cela conduit à un tas de questions lors de la révision du code. Et on demande à l'auteur du code pourquoi il a écrit ce code laid, bien qu'en fait ce code ait été simplement déplacé d'un endroit à un autre avec des changements mineurs.

Tout changement fonctionnel + tout refactoring. Nous avons déjà discuté de ce cas ci-dessus. Cela permet au réviseur de garder à l'esprit deux stratégies de révision à la fois. Même si le refactoring est bien fait, nous ne pourrons pas retarder ces modifications tant que les modifications fonctionnelles ne seront pas approuvées.

Toute modification mécanique + toute modification apportée par l'homme. Par «changements mécaniques», j'entends tout formatage effectué à l'aide de l'IDE ou de la génération de code. Par exemple, nous appliquons le nouveau style de code et obtenons 3000 modifications de ligne. Et si nous mélangeons ces modifications avec toute modification fonctionnelle ou toute autre modification apportée par une personne, nous obligerons le réviseur à classer mentalement ces modifications et la raison: il s'agit d'une modification effectuée par un ordinateur - elle peut être ignorée, et cette modification apportée par une personne est nécessaire vérifier. L'évaluateur passe donc beaucoup de temps à vérifier.

Exemple

Voici une demande de pool avec une fonction de méthode qui ferme doucement la connexion client à Memcached:



Même si la demande de pool est petite et ne contient qu'une centaine de lignes de code, elle est toujours difficile à vérifier. Comme vous pouvez le voir dans les commits, il contient différentes catégories de modifications:

• fonctionnel (nouveau code),
• refactoring (création / déplacement de classes),
• corrections de style de code (suppression des blocs de quai en excès).

En même temps, la nouvelle fonctionnalité elle-même n'est que de 10 lignes:



Par conséquent, le réviseur doit revoir l'intégralité du code et

• Vérifiez que le refactoring est OK.
• vérifier si la nouvelle fonctionnalité est correctement implémentée;
• déterminer si cette modification a été générée automatiquement par l'IDE ou par la personne.

Par conséquent, une telle demande de pool est difficile à examiner. Pour corriger la situation, vous pouvez diviser ces validations en branches distinctes et créer un pool de demandes pour chacune d'entre elles.

1. Refactoring: extraction de classe

Il n'y a que deux fichiers. Le réviseur ne doit vérifier que le nouveau design. Si tout est en ordre - merzhim.

2. L'étape suivante consiste également à refactoriser, nous déplaçons simplement les deux classes vers un nouvel espace de noms

Une telle demande de pool est assez simple à vérifier, elle peut être instanciée immédiatement.

3. Retrait des blocs de quai en excès

Rien d'intéressant ici. Merzhim.

4. La fonctionnelle elle-même

Et maintenant, la demande de pool avec des modifications fonctionnelles ne contient que le code souhaité. Votre réviseur ne peut donc se concentrer que sur cette tâche. La demande de piscine est petite et facile à vérifier.

Conclusion

Règle générale:

Ne créez pas d'énormes demandes de pool avec des catégories mixtes de modifications.

Plus la demande de pool est grande, plus il est difficile pour le réviseur de comprendre les changements que vous avez proposés. Très probablement, une énorme demande de pool avec des centaines de lignes de code sera rejetée. Au lieu de cela, divisez-le en petites parties logiques. Si votre refactoring est en ordre, mais que les modifications fonctionnelles contiennent des erreurs, le refactoring peut être facilement retenu, et vous et votre réviseur pouvez donc vous concentrer sur la fonctionnalité sans regarder tout le code depuis le tout début.

Et suivez toujours ces étapes avant d'envoyer la demande de pool:

• Optimisez votre code pour la lecture. Le code est beaucoup plus lisible qu'écrit;
• Décrire les changements proposés afin de fournir le contexte nécessaire à leur compréhension;
• vérifiez toujours votre code avant de créer une demande de pool. Et faites-le comme si c'était le code de quelqu'un d'autre. Parfois, il est utile de trouver quelque chose que vous avez manqué. Cela réduira la probabilité de rejet de votre demande de pool et le nombre de corrections.

Mon collègue Oleg Antonevich m'a parlé de l'idée de diviser les changements en catégories.

De l'éditeur: Sergey écrit beaucoup de choses intéressantes sur la programmation et PHP, et parfois nous traduisons quelque chose: un serveur vidéo en streaming , rendant des fichiers HTML . N'hésitez pas à lui poser des questions dans les commentaires de cet article - il vous répondra!

Eh bien, rappelez-vous également que nous avons toujours beaucoup de postes vacants intéressants pour les développeurs!