Frustrations de l'examen par les pairs / le code

27

Je ne m'appellerais pas un développeur superstar, mais un relativement expérimenté. J'essaie de maintenir la qualité du code à un niveau élevé et je cherche toujours à améliorer mon style de codage, à rendre le code efficace, lisible et cohérent, tout en encourageant l'équipe à suivre des modèles et des méthodologies pour assurer la cohérence. Je comprends également la nécessité d'un équilibre entre qualité et rapidité.

Pour y parvenir, j'ai présenté à mon équipe le concept de revue par les pairs. Deux pouces vers le haut dans github pull-request pour une fusion. Génial - mais pas à mon avis sans hoquet.

Je vois souvent des commentaires d'examen par les pairs des mêmes collègues comme -

  • Ce serait bien d'ajouter un espace après <INSERT SOMETHING HERE>
  • Ligne supplémentaire indésirable entre les méthodes
  • Le point final doit être utilisé à la fin des commentaires dans les docblocks.

Maintenant, de mon point de vue - le critique examine superficiellement l'esthétique du code - et n'effectue pas vraiment de révision du code. La révision du code cosmétique me semble être une mentalité arrogante / élitiste. Il manque de substance, mais vous ne pouvez pas trop en discuter car le critique est techniquement correct . Je préférerais de beaucoup voir moins de types de critiques ci-dessus, et plus de critiques comme suit:

  • Vous pouvez réduire la complexité cyclomatique en ...
  • Sortez tôt et évitez si / sinon
  • Résumé de votre requête DB dans un référentiel
  • Cette logique n'a pas vraiment sa place ici
  • Ne vous répétez pas - résumé et réutilisation
  • Que se passerait-il s'il Xétait passé comme argument à la méthode Y?
  • Où est le test unitaire pour cela?

Je trouve que ce sont toujours les mêmes types de personnes qui donnent les évaluations cosmétiques et les mêmes types de personnes qui, à mon avis, donnent les évaluations par les pairs "basées sur la qualité et la logique".

Quelle est (le cas échéant) la bonne approche de l'examen par les pairs. Et ai-je raison d'être frustré par les mêmes personnes qui survolent essentiellement le code à la recherche d'erreurs d'orthographe et de défauts esthétiques plutôt que de véritables défauts de code?

Si je ne me trompe pas - comment pourrais-je encourager mes collègues à rechercher des défauts dans le code, tout en suggérant des retouches cosmétiques?

Si je me trompe, veuillez m'éclairer. Existe-t-il des règles générales pour déterminer ce qui constitue réellement une bonne révision de code? Ai-je manqué de savoir ce que sont les revues de code?

De mon point de vue - la révision du code concerne la responsabilité partagée du code. Je ne me sentirais pas à l'aise de donner le feu vert au code sans adresser / vérifier la logique, la lisibilité et la fonctionnalité. Je ne prendrais pas non plus la peine de bloquer une fusion pour un morceau de code solide si je remarquais que quelqu'un avait omis un point dans un bloc doc.

Lorsque je passe en revue le code, je passe peut-être entre 15 et 45 minutes pour 500 loc. Je ne peux pas imaginer que ces évaluations superficielles prennent plus de 10 minutes jamais si c'est la profondeur de l'examen qu'elles effectuent. De plus, quelle est la valeur du pouce levé de l'examinateur superficiel? Cela signifie sûrement que tous les pouces n'ont pas le même poids et qu'il doit peut-être y avoir un processus d'examen en deux passes. Un pouce pour les critiques approfondies et un 2e pouce pour le "polissage"?

Sauce
la source
2
Avez-vous essayé de le mentionner à cette personne?
Bryan Oakley
11
J'avais un supérieur qui nécessitait des arrêts complets dans tous les commentaires, ainsi qu'une capitalisation et une ponctuation et une grammaire appropriées. Il était également très anal sur les espaces blancs. Cela m'a rendu fou, mais cela a également conduit à un code cohérent très lisible.
Bryan Oakley
4
Les trois premières puces de votre message sont des pertes de vélos, sauf si elles font partie d'une norme de magasin de style de codage. Si vous écrivez de la documentation, je m'attendrais à une orthographe et une grammaire parfaites. Ce n'est pas difficile à réaliser de nos jours, étant donné l'abondance de vérificateurs orthographiques et de vérificateurs de grammaire dans les traitements de texte. De même, les problèmes de style de codage peuvent souvent être automatisés dans des éditeurs de code suffisamment intelligents. Je ne m'attendrais pas à voir ce genre de choses dans une revue de code; le temps de chacun est trop précieux.
Robert Harvey
2
l'examen arrogant / élitiste est facile et ne nécessite presque aucun effort. Pour faire un examen technique, vous devez lire et comprendre le code, puis penser à une meilleure solution ... cela signifie que vous devez travailler. Je ne suis pas surpris du résultat de votre proposition. Vous devez organiser le processus d'examen avec des tâches mesurables et bien définies pour réaliser quelque chose.
JoulinRouge
2
Si vous utilisez Agile, c'est quelque chose que vous pourriez évoquer lors de rétros sans indiquer une seule cible. Mentionnez que la fonction principale des revues de code est l'exactitude du code, et non l'esthétique du code. Dans ce cas, cela devient un «besoin de changer», et quelque chose que vous pouvez continuellement évoquer jusqu'à ce qu'il change réellement.
Canadian Coder

Réponses:

22

Types d'avis

Il n'y a pas de véritable moyen de faire des évaluations par les pairs. Il existe de nombreuses façons de juger si le code est d'une qualité suffisamment élevée. Il y a clairement la question de savoir s'il s'agit d'un buggy, ou s'il a des solutions qui ne sont pas évolutives ou qui sont fragiles. Les problèmes de conformité aux normes et directives locales, bien qu'ils ne soient peut-être pas aussi critiques que certains autres, font également partie de ce qui contribue à un code de haute qualité.

Types d'examinateurs

Tout comme nous avons des critères différents pour juger les logiciels, les personnes qui font le jugement sont également différentes. Nous avons tous nos propres compétences et prédilections. Certains peuvent penser qu'il est très important de respecter les normes locales, tout comme d'autres peuvent être plus préoccupés par l'utilisation de la mémoire ou la couverture du code de vos tests, etc. Vous voulez tous ces types d'avis, car dans l'ensemble, ils vous aideront à écrire un meilleur code.

Un examen par les pairs est une collaboration, pas un jeu de tag

Je ne suis pas sûr que vous ayez le droit de leur dire comment faire leur travail. À moins que vous ne sachiez le contraire avec certitude, supposez que cette personne essaie de contribuer comme elle l'entend. Cependant, si vous voyez des améliorations à apporter ou si vous pensez qu'ils ne comprennent peut-être pas ce que l'on attend d'une évaluation par les pairs, parlez-leur .

Le point d'un examen par les pairs est d' impliquer vos pairs . La participation ne lance pas de code sur un mur et n'attend pas qu'une réponse soit renvoyée. La participation consiste à travailler ensemble pour créer un meilleur code. Engagez une conversation avec eux.

Conseil

Vers la fin de votre question, vous avez écrit:

Comment pourrais-je encourager mes collègues à rechercher réellement des défauts dans le code en équilibre avec des erreurs esthétiques flagrantes?

Encore une fois, la réponse est la communication. Peut-être que vous pouvez leur demander "hé, je vous remercie d'avoir rattrapé ces erreurs. Cela m'aiderait énormément si vous pouviez également vous concentrer sur des questions plus profondes telles que la structuration correcte de mon code. Je sais que cela prend du temps, mais cela serait vraiment Aidez-moi."

Sur une note plus pragmatique, je divise personnellement les commentaires de révision de code en deux camps et les formule de manière appropriée: les choses qui doivent être corrigées et les choses qui sont plus cosmétiques. Je n'empêcherais jamais un code solide et fonctionnel d'être archivé s'il y avait trop de lignes vides à la fin d'un fichier. Je le ferai remarquer, cependant, mais je le ferai avec quelque chose comme "nos directives disent d'avoir une seule ligne vierge à la fin, et vous en avez 20. Ce n'est pas un spectacle, mais si vous en avez l'occasion, vous pourrait vouloir le réparer ".

Voici autre chose à considérer: il se peut que ce soit une bête noire que vous fassiez un examen aussi superficiel de votre code. Il se peut très bien qu'une bête noire de leur est que vous (ou un autre coéquipier qui obtient un examen similaire) sont bâclée par rapport aux normes de codage de votre propre organisation, ce qui est la façon dont ils ont choisi de communiquer avec vous.

Que faire après l'examen

Et enfin, un petit conseil après la révision: lors de la validation du code après une révision, vous voudrez peut-être envisager de prendre soin de toutes les choses cosmétiques dans un commit et des changements fonctionnels dans un autre. Le mélange des deux peut rendre difficile la différenciation des changements significatifs des changements insignifiants. Effectuez toutes les modifications esthétiques, puis validez avec un message du type "cosmétique; aucune modification fonctionnelle".

Bryan Oakley
la source
Merci pour une excellente réponse. Je suppose que mes frustrations résident dans le fait que les biggerproblèmes ne sont pas résolus. Tels que les index manquants sur une table DB. Ou tenter d'utiliser une méthodologie ou un algorithme sans comprendre le raisonnement et, en tant que tel, le faire de manière incorrecte. Pour moi - ce sont plus importants et doivent être abordés et résolus principalement - avec l'esthétique étant une préoccupation secondaire.
Gravy
1
savez-vous que des problèmes plus importants sont manqués ou avez-vous simplement peur qu'ils le fassent? Lorsqu'un gros problème est manqué, lors d'une rétrospective ou d'une réunion d'équipe, vous pouvez indiquer que ce sont les types de choses qui devraient être détectées lors de la révision du code. Vous pourriez même envisager de demander qui dans l'équipe se concentre sur les changements de base de données, et si personne ne lève la main, peut-être en nommer certains pour essayer de se concentrer uniquement sur les changements de base de données pour la prochaine revue.
Bryan Oakley
Pour être honnête, la majorité des commentaires cosmétiques ne visent généralement pas mon code. Quand je passe en revue le code des autres, je vois des commentaires contre les RP concernant les cosmétiques, juste à côté du code que je considérerais comme un gros problème. De plus, une grande partie de la langue maternelle de l'équipe n'est pas l'anglais. Donc, de mon point de vue - l'étrange erreur d'orthographe / grammaire est pardonnable. Je ne suis pas ici pour revoir leur utilisation de l'anglais dans un commentaire doc-block, mais leur code.
Gravy
2
Eh bien, leurs commentaires font partie du code source, et s'ils sont inutilement difficiles à analyser, trompeurs ou même erronés, les avoir peut être un mauvais service pour quiconque a plus tard l'occasion de consulter ce code pour une raison quelconque. Il n'est pas nécessaire qu'ils soient formels ou des œuvres d'art pour atteindre leur objectif, mais un anglais cassé, quelle que soit la maîtrise de la langue de l'auteur, entrave l'objectif. Mieux vaut n'avoir aucun commentaire que les mauvais.
Déduplicateur
1
La plupart des gens apprécient que des erreurs de langage leur soient signalées afin qu'elles puissent s'améliorer.
gnasher729
7

Les gens commentent le formatage du code et les fautes de frappe, car ils sont faciles à repérer et ne nécessitent pas beaucoup d'efforts de leur part.

Cette partie est facile à corriger - presque toutes les langues ont un outil de vérification de style ou de linter. Branchez-le dans votre processus de génération, afin qu'il échoue à la génération s'il y a un espace redondant. Par conséquent, il n'y aura aucun problème de style à commenter.

Les amener à trouver de vrais problèmes pourrait être tout un défi. Non seulement cela nécessite un état d'esprit curieux et axé sur les détails, mais aussi une motivation importante.

Et cette motivation vient de l'expérience. Expérience d'essayer de travailler avec du code merdique écrit par des développeurs précédents. Les ingénieurs seniors en ont beaucoup. Nager dans l'océan de merde vous donne tout à fait envie de ne plus y retourner.

Si je dois choisir une chose principale à vérifier lors de la révision du code, ce serait la maintenabilité du code. À tout moment, le développeur qui examine ce code doit le comprendre et être prêt à l'améliorer et à le modifier. S'il n'a pas la moindre idée de ce qui se passe ici, il doit le dire tout de suite et le code doit être réécrit.

alex
la source
Je suis d'accord avec vous sur vos commentaires. Et si vous voyez cela ocean of shitécrit par moi - alors je vous encourage à suggérer que je réécris. Mais si vous ignorez le shitmais me demande de réparer les trucs cosmétiques - c'est ce qui me frustre.
Gravy
4

Voici la réponse pratique:

Scénario 1 : vous êtes le membre senior et quelqu'un qui peut décider de la pratique

Convoquez une réunion et exposez les règles et les directives de révision du code. Croyez-moi, des directives claires fonctionnent mieux que tout système ou formation «d'honneur». Indiquez clairement que les problèmes de formatage du code ne doivent pas être soulevés du tout. Certains membres s'y opposeront. Écoutez-les, puis demandez-leur de suivre les directives pendant les premiers mois à titre expérimental. Faites preuve de volonté de changer si les directives actuelles ne fonctionnent pas.

Scénario 2 : vous n'êtes pas quelqu'un qui décide de la pratique ou vous êtes un membre relativement junior de l'équipe

Votre meilleur pari est de simplement résoudre les problèmes de révision de code jusqu'à ce que vous parveniez à atteindre le scénario 1 .

Kshitij Upadhyay
la source
2

La réponse simple pour éviter les commentaires de révision de code "triviaux" consiste à insister (par souci d'efficacité) sur le fait que c'est le réviseur qui doit les corriger. Donc, si le critique constate que vous avez (horreur!) Manqué un arrêt complet ou épelé un commentaire à tort, il doit simplement le corriger et passer à autre chose.

D'après mon expérience, cela signifie que:

  • vos réviseurs cessent de faire ces commentaires de révision relativement inutiles et de les transmettre pour être corrigés.
  • seuls les réviseurs les plus OCD les corrigeront, ce qui signifie que la plupart de vos avis réussiront. cela a pour effet de forcer les développeurs à effectuer des examens plus substantiels, ceux qui signalent des anecdotes parce qu'ils ne révisent pas réellement le code finiront par devoir justifier pourquoi tous leurs avis sont passés sans commentaire.

Quoi qu'il en soit, c'est un avantage. Le coût d'un examen échoué est élevé en termes de faire arrêter un développeur sur ce qu'il travaille et de revoir son code, et l'examen de suivi ultérieur. Si une revue trouve de vrais problèmes de qualité de code ou d'architecture, alors ce coût vaut chaque centime, mais le payer pour des anecdotes ne l'est pas.

gbjbaanb
la source
0

Examiner le processus d'examen

En plus des révisions de code, je suggérerais également de revoir régulièrement le processus de révision. Il y a certainement beaucoup de choses que les gens peuvent aussi apprendre ici, par exemple comment donner des revues de code appropriées.

Habituellement, certains des cyclistes sont inexpérimentés et ne savent tout simplement pas quoi chercher. Ils ont besoin d'un peu de conseils non seulement en ce qui concerne leur code, mais aussi en ce qui concerne les révisions de code utiles. Fournir des commentaires sur les avis conduira à un processus d'apprentissage qui (plein d'espoir) se traduira par de meilleurs avis et un meilleur code.

Ensuite, ce pourrait être une bonne idée de formaliser (vaguement) un ensemble de valeurs et de critères, ce que l'organisation ou l'équipe perçoit comme un bon code ™, et quels sont les anti-modèles à éviter. Il ne s'agit pas de définir qc. dans le béton, mais à obtenir un consensus commun à propos de la qualité du code depuis le début .

JensG
la source
0

En tant que personne qui a été des deux côtés (en examinant le code écrit par d'autres, ainsi qu'en faisant réviser le code que j'ai écrit), je dirais que j'ai trois catégories dans lesquelles tous les commentaires entrent. Eh bien, quatre, il y a aussi le délicieux cas de "tout va bien".

"Ce serait bien, mais cela ne vous bloquera pas" (si tous les commentaires entrent dans cette catégorie, je peux envoyer la demande de fusion avec un "fusionnera à XX: XX, sauf si vous me dites que vous voulez les corriger" , ou "bien sûr, allez-y pour vous enregistrer, quel que soit le bloc que le système lancerait a été désactivé"):

  • Oublier les arrêts complets à la fin des phrases (dans les chaînes de doc ou les commentaires). Grammaire maladroite dans tout ce qui n'est pas émis aux utilisateurs sous quelque forme que ce soit (encore une fois, les chaînes de doc et les commentaires)
  • Code qui a une manière plus élégante, mais ce qu'il y a est compréhensible et idiomatique (je vais probablement même y mettre ma suggestion, il est donc facile de laisser ou de corriger)

"Les choses qui doivent être corrigées, mais je vais vous faire confiance pour le faire" (si toutes les choses appartiennent à cette catégorie ou à la précédente, je répondrai par "corrigez-les, je fusionnerai quand vous me direz que vous" re done "(et à ce moment-là je vais probablement rapidement scanner pour voir que rien d'autre n'est apparu)):

  • Petits problèmes ("vous comparez un booléen à true, c'est un peu paranoïaque ...", ...)
  • Violations mineures de style ("le guide de style dit X, ce que vous avez fait est en dehors de cela; je ferais Y ou Z, selon la direction que vous voulez prendre")
  • Quelques cas de test manquants, qui ne devraient pas être difficiles à écrire

Et enfin, "les choses qui sont problématiques, je devrai revoir votre code une fois que vous aurez corrigé ces problèmes" (s'il y en a dans cette catégorie, il devra y avoir une autre série de révisions, alors mettez un commentaire disant " il y a aussi quelques petites choses, ce serait bien de voir certaines d'entre elles corrigées pendant que vous y êtes "s'il y a quelque chose des deux premières catégories présentes):

  • Ce serait des choses comme «non, il y a vraiment une bien meilleure façon d'écrire cela», «vous ne calculez pas ce que vous attendez, car votre test unitaire manque ces cas limites» et tous les autres problèmes graves.
Vatine
la source
0

Il semble que certaines personnes ont oublié la question la plus importante: qu'est-ce que vous voulez réellement réaliser avec les revues de code?

Le but des revues de code est d'obtenir un code sans bogue et maintenable plus rapidement. Les révisions de code y parviennent de plusieurs manières: les développeurs écrivent un meilleur code en premier lieu parce qu'ils savent qu'il sera révisé, les connaissances sont partagées dans le cadre du processus de révision, des bogues seront détectés parce que le réviseur n'est pas aveugle à leurs propres erreurs en tant que développeurs peut être.

Si vous voyez le processus de révision comme une chance d'abattre vos collègues ou de créer du travail pour eux, alors vous le faites mal.

gnasher729
la source