Comment effectuer la révision du code par Google

Je m'intéresse aux questions de révision de code depuis très longtemps. Plusieurs fois, un problème ou un autre s'est posé, soit avec la qualité du code, soit avec le climat dans l'équipe. En effet, la revue de code est sinon le seul, alors l'un des endroits les plus importants pour les conflits dans l'équipe de développement.

Et récemment, en préparation de la prochaine version du podcast Zinc Prod , je découvre que Google a publié une directive de révision de code pleine de réflexions précieuses. Tout le matériel est assez volumineux et ne rentrera pas dans un seul article, je vais donc essayer de mettre en évidence les pensées les plus intéressantes (pour moi).

Alors allons-y

Termes utilisés dans l'article d'origine:

CL - liste des modifications. Mais généralement, cela s'appelle Merge Request ou Pull Request, donc dans l'article j'utiliserai l'abréviation MR

LGTM - Ça m'a l'air bien. Bref, quand ils appuient sur le bouton "approuver". J'utiliserai le terme "aprov", comme plus compréhensible pour la population.

Idéalité mr

Dans la pratique, on peut rencontrer différents extrêmes lorsque l'on considère la RM. Quelqu'un dans son ensemble commente zadalivaet, jusqu'à ce que tout soit insignifiant, soit corrigé, quelqu'un regarde la logique et appuie sur "upruv".

Une pensée intéressante est écrite dans le document Google. MR ne doit pas être parfait, mais il devrait améliorer la base de code. Autrement dit, à chaque changement introduit, le code devrait s'améliorer de mieux en mieux. Et si MR ajoute beaucoup de bien, alors vous n'avez pas à vous plaindre des petites choses; il est plus rentable d'obtenir cette amélioration plus rapidement.

Aucun MR ne devrait aggraver le code. La seule exception est si MR est une solution urgente pour quelque chose.

Liberté de commenter

Malgré le fait que vous ne pouvez pas vous débarrasser des bagatelles, vous pouvez néanmoins écrire librement ces petites choses sur au moins chaque ligne. La contradiction avec le paragraphe précédent est résolue simplement: le réviseur préfixe les bagatelles et la pioche avec le préfixe "nit:" de l'anglais. piqûre (piqûre). Il n'est pas nécessaire de corriger de tels commentaires, cependant, l'auteur de MR peut vouloir corriger quelque chose ou, même si ce n'est pas le cas, prendre en compte certains points pour l'avenir.

Faits sur les préférences personnelles

Presque toujours, les principes de conception de logiciels standard, basés sur les meilleures pratiques, sont meilleurs que les vélos difficiles. Par conséquent, la préférence devrait leur être accordée.
Si vous pouvez appliquer plusieurs approches standard, cela est à la discrétion de l'auteur.
Devis direct pour une meilleure compréhension:

Les aspects de la conception de logiciels ne sont presque jamais un problème de style pur ou simplement une préférence personnelle. Ils sont fondés sur des principes sous-jacents et doivent être pesés sur ces principes, et pas seulement par une opinion personnelle. Parfois, il existe quelques options valides. Si l'auteur peut démontrer (soit par des données, soit sur la base de principes d'ingénierie solides) que plusieurs approches sont également valables, alors le réviseur doit accepter la préférence de l'auteur. Sinon, le choix est dicté par les principes standard de la conception de logiciels.

Si le critique et l'auteur de MR ne sont pas d'accord

Vient d'abord une tentative de parvenir à un consensus dans les commentaires sur MR. Si cela ne fonctionne pas, alors une discussion personnelle. Si vous n'arrivez toujours pas à un consensus, alors attirez les membres de l'équipe. Mais le plus important, MR ne devrait pas être bloqué pendant une longue période en raison du désaccord de deux personnes.

Discuté dans les commentaires -> Discuté personnellement -> Discuté en équipe -> Aller de l'avant

Vitesse de vérification MR

La vitesse est extrêmement importante. Si vous donnez un commentaire tous les quelques jours, alors l'auteur de MR se plaindra qu'ils lui reprochent et l'empêchent de travailler.

Si MR est vérifié très rapidement, tous les commentaires ne seront pas un gros problème - après tout, leurs correctifs seront vérifiés ici et la tâche se poursuivra.

Google vous conseille de ne pas être distrait de se concentrer sur votre tâche, mais si vous êtes distrait, alors voyez s'il y a quelque chose à réviser. Par exemple, il est revenu du déjeuner - il a révisé. Entré au travail - révisé. Etc.

Ordre de visualisation MR

Vous devez d'abord regarder le MR dans son ensemble. Est-il nécessaire du tout, le code est-il au bon endroit (ou devrait-il être dans une bibliothèque séparée), y a-t-il des problèmes globaux?
C'est-à-dire cela n'a aucun sens de regarder certains détails d'implémentation si le code n'est pas là et pas du tout.

En général, l'ordre de visionnement est important afin de donner un retour à l'auteur dans les plus brefs délais (voir paragraphe précédent).

Lorsque vous avez examiné MR dans son ensemble, vous devez parcourir les fichiers principaux, c'est-à-dire vérifiez la chose la plus importante (si ce n'est pas clair ce qui est le plus important, vous pouvez demander au développeur).

Encore une fois, tout problème doit être signalé immédiatement, même si vous n'avez pas encore inspecté le MR et décidé de l'inspecter généralement plus tard.

Ensuite, vous devez sélectionner une séquence logique pour afficher les fichiers restants. Aucun fichier ne doit être ignoré.

Que rechercher lors de la visualisation

• Le code est bien conçu
• La fonctionnalité est bien faite du point de vue des utilisateurs de ce code, peu importe qui ils sont.
• L'apparence (le cas échéant) devrait être bonne
• Toutes les nuances de la programmation parallèle (le cas échéant) sont prises en compte.
• Code non repensé
• Le développeur ne fait pas trop de travail: vous n'avez pas besoin d'écrire du code qui peut être nécessaire, ou qui peut ne pas être nécessaire
• Le code a des tests
• Les tests sont bien conçus
• Les noms (pour tout) sont bien choisis
• Les commentaires sur le code sont compréhensibles et nécessaires. Ils doivent expliquer pourquoi cela est fait et non pas comment cela se fait.
• Ajout de documentation.
• Le code correspond aux guides de style.

Trop grand MR

Trop de MR doivent être demandés pour être démantelés. C'est presque toujours possible.

Comment écrire des commentaires sur la révision du code

• Vous devez être poli, pas pour devenir personnel. Discutez du code, pas du codeur.
• Pas seulement émettre des directives pour les corrections, mais expliquer pourquoi vous devez les corriger.
• Maintenir un équilibre: identifier le problème et pousser le développeur afin qu'il comprenne lui-même la meilleure façon de le résoudre; ou émettez immédiatement une solution toute faite. Le premier développe le développeur (bénéfice stratégique), le second améliore et accélère le MR (bénéfice tactique).
• Si le réviseur n'a pas compris à un moment donné du code et demande à l'auteur d'expliquer quoi, alors la meilleure réponse serait de changer le code. Alors que tout était clair du code sans aucun doute.

Si vous n'êtes pas d'accord avec votre opinion

Il est nécessaire de comprendre en détail. Peut-être que vous ne comprenez pas quelque chose, demandez plus d'informations. Peut-être le contraire. Dans tous les cas, la compréhension ne vient souvent qu'après quelques séries d'explications des deux côtés.

Peur de bouleverser l'auteur MR

Il arrive que le développeur soit contrarié si le réviseur insiste sur certains changements. Cependant, le plus souvent, les développeurs sont contrariés à cause de la forme du commentaire, et non à cause du contenu. Soyez poli, expliquez avec des arguments, et très probablement tout ira bien.

"Je vais le réparer plus tard."

Si le développeur convient qu'il y a un problème dans le code, mais demande MR, promettant qu'il le réglera une autre fois, alors, de l'avis des gars de Google, ce "plus tard" ne se produit le plus souvent jamais. Par conséquent, si MR n'est pas un correctif de bogue urgent, vous devez insister sur un correctif.

Conclusions

J'ai vraiment aimé ce document de Google. Surtout la vie hack avec le mot "nit", l'accent mis sur la vitesse de révision du code, et aussi que MR ne devrait pas être parfait. Cela semble simple, mais tant que cela n'est clairement pas pris en compte, cela n'atteint pas le but.

Si cet article semble intéressant pour les lecteurs et recueille un certain nombre d'avantages, j'écrirai la partie suivante: le processus de révision du code par l'auteur MR.

Mise à jour: la deuxième partie de l'article ici

De plus, dans le prochain numéro de Zinc Sale, nous discuterons en détail du code de révision sous différents angles. Assurez-vous donc de vous abonner à notre podcast de développement!