Dans les revues de code, le relecteur devrait-il toujours présenter une solution aux problèmes? [fermé]

93

Lors de l'examen du code, j'essaie normalement de faire des recommandations spécifiques sur la façon de résoudre les problèmes. Mais vu le peu de temps que l’on peut consacrer à l’examen, cela ne fonctionne pas toujours bien. Dans ces cas, je trouve plus efficace que le développeur propose lui-même une solution.

Aujourd'hui, j'ai passé en revue un code et constaté qu'un cours n'était évidemment pas bien conçu. Il comportait un certain nombre d'attributs facultatifs qui n'étaient attribués que pour certains objets et laissés en blanc pour d'autres. La méthode standard pour résoudre ce problème consiste à scinder la classe et à utiliser l'héritage. Cependant, dans ce cas précis, cette solution a semblé compliquer les choses. Je n'ai pas participé moi-même au développement de ce logiciel et je ne connais pas tous les modules. Par conséquent, je ne me sentais pas suffisamment informé pour prendre une décision spécifique.

Un autre cas typique que j’ai souvent expérimenté est le fait que je trouve une fonction, un nom de classe ou une variable évidemment dépourvu de sens ou même trompeur, mais que je ne parviens pas à me nommer moi-même.

Donc, généralement, en tant que critique, est-il correct de dire "ce code est défectueux parce que ..., faites-le différemment" ou devez-vous proposer une solution spécifique?

Frank Puffer
la source
24
@gnat: Non, le code n'est pas trop compliqué. Et ce n'est qu'un exemple. Je demande généralement si l'examinateur est responsable de la présentation d'une solution.
Frank Puffer
5
non, je dirais qu'en tant que critique, vous n'êtes pas obligé de dire comment l'améliorer. Si vous pouvez décrire ce qui ne va pas, faites-le. sinon, fournissez simplement un commentaire général. (L'un des commentaires les plus utiles que je me souvienne d'avoir reçu était littéralement du type "cette classe est une ordure totale")
gnat
5
"Le moyen standard pour résoudre ce problème serait de scinder la classe et d'utiliser l'héritage." J'espère bien que vous ne révisez pas mon code!
Gardenhead
7
Identifier les problèmes potentiels pourrait suffire. Le réviseur examine le code en tant qu'utilisateur, responsable ou concepteur. Le codeur n’a peut-être pas encore remarqué la présence d’un angle de vue différent ou la possibilité de détecter des problèmes, ce qui peut l’aider à améliorer son travail. Et si vous remarquez quelque chose que vous aimez, il ne serait pas mal de le signaler aussi. Ce ne devrait pas être un exercice correctif, mais plutôt un exercice éclairant. C'est pourquoi on parle de "peer review".
Martin Maat

Réponses:

139

En tant que réviseur, votre travail consiste à vérifier si un morceau de code (ou un document) répond à certains objectifs convenus avant la révision.

Certains de ces objectifs impliquent généralement un jugement qui détermine si l'objectif a été atteint ou non. Par exemple, l'objectif que le code doit pouvoir être mis à jour nécessite généralement un appel de jugement.

En tant qu’examinateur, votre travail consiste à indiquer où les objectifs n’ont pas été atteints et à l'auteur de veiller à ce que son travail réponde réellement aux objectifs. De cette façon, il ne vous appartient pas de dire comment les corrections doivent être apportées.

D'autre part, le simple fait de dire à l'auteur que "c'est imparfait. Corrigez-le" ne crée généralement pas une atmosphère positive dans l'équipe. Pour une atmosphère positive, il est bon d’indiquer au moins pourquoi quelque chose ne va pas dans vos yeux et de fournir une meilleure alternative si vous en avez une.
En outre, si vous passez en revue quelque chose qui semble "faux" mais que vous n'avez pas vraiment de meilleure alternative, vous pouvez également laisser un commentaire dans le sens de "Ce code / cette conception ne me convient pas, mais je ne pas avoir une alternative claire. Pouvons-nous en discuter? " et ensuite essayer d'obtenir quelque chose de mieux ensemble.

Bart van Ingen Schenau
la source
23
+ 1 pour discussion afin de trouver une solution ensemble - c’est ainsi que j’apprends le plus des programmeurs
expérimentés
19
+1 Lorsque vous commentez, il est préférable de formuler des critiques constructives chaque fois que possible.
FrustratedWithFormsDesigner
7
Je suis d'accord avec le dernier bit en particulier. Il est parfaitement correct de dire "cette solution ne va pas parce que ..." ou "je crains que cette partie ne pose problème car ..." sans donner de solution.
Daniel T.
1
@ doancoo: Le "pouvons-nous discuter de cela" était destiné à être une question. Personnellement, j'aurais de toute façon la discussion, même si ce n'est que pour apprendre quelque chose moi-même.
Bart van Ingen Schenau
1
Le danger subtil réside dans le fait que, avec suffisamment de discussions et de communications sur la mise en œuvre, cela cesse d’être une révision et devient une programmation en binôme. La programmation en binôme est bonne, mais une fois que c'est fait, vous avez besoin d'une 3ème personne à réviser car l'indépendance a été perdue.
candied_orange
35

Quelques bonnes réponses ici, mais je pense qu'il manque un point important. Le code que vous révisez, l'expérience de cette personne et la façon dont elle traite de telles suggestions fait une grande différence. Si vous connaissez bien votre coéquipier et que vous vous attendez à une note du type "ce code est défectueux car ..., faites-le différemment" pour lui permettre de proposer une meilleure solution, alors un tel commentaire peut convenir. Mais il y a certainement des personnes pour lesquelles un tel commentaire n'est pas suffisant et qui doivent savoir précisément comment améliorer leur code. Donc, à mon humble avis, c’est un jugement que vous ne pouvez prendre que pour chaque cas individuel.

Doc Brown
la source
29

Donc en général, en tant que critique, est-il correct de dire "ce code est défectueux, faites-le différemment" ou devez-vous proposer une solution spécifique?

Aucune des deux n'est idéale IMO. La meilleure chose à faire est de parler à l'auteur et de résoudre le problème en collaboration.

Les revues de code ne doivent pas nécessairement être asynchrones. De nombreux problèmes seront résolus si vous cessez de les considérer comme un processus bureaucratique et prenez un peu de temps pour communiquer en direct.

guillaume31
la source
"Processus bureaucratique" est un bon moyen de le dire!
frnhr
17

Dans les revues de code, le relecteur devrait-il toujours présenter une solution aux problèmes?

Non. Si vous faites cela, vous n'êtes pas un critique, vous êtes le prochain codeur.

Dans les revues de code, le relecteur ne devrait-il jamais présenter une solution aux problèmes?

Non, votre travail consiste à communiquer le problème en cause. Si présenter une solution rend le problème clair, alors faites-le. Ne vous attendez pas à ce que je suive votre solution. La seule chose que vous avez à faire ici est de faire valoir un point. Vous ne devez pas dicter la mise en œuvre.

Quand un relecteur doit-il présenter une solution aux problèmes?

Quand c'est le moyen le plus efficace de communiquer. Nous sommes des singes de code, pas des majors anglais. Parfois, la meilleure façon de montrer que le code est nul ... n'est pas optimal ... est de leur montrer un code qui est moins efficace ... est plus optimiste ... ah bon Dieu, vous voyez ce que je veux dire.

confits_orange
la source
8
Ne codez pas dans le vide, parce que ça craint.
Hm. Lorsque je suggère une solution à un problème, cela présente souvent des avantages dont je suis conscient, mais il me faudrait tout simplement trop de temps pour en dresser une liste exhaustive. (Celles-ci sont souvent liées à la stabilité et à la certitude que la chose va continuer à fonctionner pendant que nous changeons de choses.) Donc, si vous faites autre chose qui ne résout pas autant de problèmes, je ne serais pas tout à fait heureux (au moins pas à moins que vous puissiez me donner une bonne raison pour laquelle ce que j'ai suggéré n'a pas fonctionné). Comment géreriez-vous cela?
jpmc26
1
PS: "Code singe" est souvent utilisé pour décrire un programmeur non qualifié et irréfléchi qui fait simplement ce qu'on lui dit, même si c'est une mauvaise idée et qu'il n'a pas une bonne sensibilité de conception. Voir Dictionnaire urbain . Même Wikipedia note que c'est parfois péjoratif.
jpmc26
@ jpmc26 si vous allez utiliser du code pour communiquer avec moi, j'espère que vous utiliserez un code qui montre comment le problème pourrait être mieux résolu. De plus, Code Monkey peut être utilisé avec affection. Certes, plus d'affection que les majors anglais
candied_orange
"Code singe me lève prendre un café. Code singe va au boulot. Code singe ont une réunion ennuyeuse, avec le manager ennuyeux Rob. Rob dit que le singe code est très diligent, mais sa sortie pue ..."
Baldrickk
13

Le problème principal est que si les gens savaient mieux écrire le code, ils l'auraient généralement fait en premier lieu. Si une critique est suffisamment spécifique dépend beaucoup de l'expérience de l'auteur. Les programmeurs très expérimentés pourront peut-être accepter des critiques du type "cette classe est trop compliquée", revenir à la table des matières et proposer quelque chose de mieux, car ils avaient juste une journée de congé à cause d'un mal de tête ou étaient négligents parce qu'ils Pressé.

Habituellement, vous devez au moins identifier la source de la complication. "Cette classe est trop compliquée car elle enfreint la loi de Demeter partout." "Cette classe mélange les responsabilités des couches de présentation et de persistance." Apprendre à identifier ces raisons et à les expliquer succinctement fait partie du processus de devenir un meilleur examinateur. Vous devez rarement entrer dans les détails des solutions.

Karl Bielefeldt
la source
4
+100 Ce que je ressens le plus souvent avec Code Reviews est que si j'avais su trouver un meilleur moyen, j'aurais probablement écrit de cette façon.
RubberDuck
J'aime ta première phrase. Cela m'a fait penser à me demander: "ce code est-il suffisant?" puis lancez une pièce pour décider si vous voulez l’améliorer ou non! (Normalement, je reste juste jusqu'à ce que je manque de temps, mais je pourrais peut-être m'arrêter quand ce sera assez bien à la place?)
OMI "Ce code est compliqué car il enfreint la loi de Demeter" est un mauvais commentaire. "Ce code est compliqué car X est trop couplé à Y et Z" est meilleur.
user253751
"Si les gens savaient mieux écrire le code, ils l'auraient généralement fait en premier lieu". Il y a des exceptions. J'ai développé ce code ce genre de travail mais nous mordrons dans le cul à l'avenir. Le responsable non technique ne comprend pas "Je n'aime pas ce code et souhaite trois jours pour l'améliorer". Le responsable non technique comprend "Joe a examiné et rejeté ce code et il me faut trois jours pour l’améliorer".
gnasher729
4

Il existe deux types de mauvais programmeurs: ceux qui ne suivent pas les pratiques standard et ceux qui "ne font" que suivre les pratiques standard.

Quand j'ai eu un contact de travail limité / fournir des commentaires à quelqu'un, je ne dis pas: "C'est un mauvais design." mais quelque chose comme "Pouvez-vous m'expliquer ce cours?" Vous découvrirez peut-être que c'est une bonne solution, le développeur a sincèrement fait de son mieux, ou même reconnu qu'il s'agissait d'une mauvaise solution, mais c'est suffisant.

En fonction de la réponse, vous aurez une meilleure idée de la façon d'aborder chaque situation et chaque personne. Ils peuvent rapidement reconnaître le problème et découvrir eux-mêmes le correctif. Ils peuvent demander de l'aide ou vont simplement essayer de résoudre le problème par leurs propres moyens.

Il existe des pratiques suggérées dans notre entreprise, mais elles ont presque toutes des exceptions. Si vous comprenez le projet et la façon dont l'équipe l'aborde, cela peut être le contexte pour déterminer l'objectif de la révision du code et la manière de résoudre les problèmes.

Je me rends compte qu’il s’agit plus d’une approche du problème que d’une solution explicite. Il y aura trop de variabilité pour couvrir toutes les situations.

JeffO
la source
1
Mais, si cela nécessite une explication pour que la conception soit reconnaissable, il manque des commentaires en ligne.
Wildcard
1
Parfois, les règles ne font pas exception, mais généralement pas.
@Wildcard - cela dépend des capacités et des préférences / opinions des personnes qui la regardent.
JeffO
1
@Wildcard J'accepte l'approche selon laquelle les commentaires devraient être formulés sous forme de question, mais la réponse prendra (éventuellement) la forme d'un commentaire, ou peut-être d'un changement de code (par exemple, une meilleure dénomination). Cela laisse la porte ouverte au développeur qui peut expliquer son raisonnement et discuter des options plutôt que de se sentir comme une demande ou d'accomplir son travail par inadvertance.
IMSoP
3

Lorsque j'examine le code, je n'apporte une solution aux problèmes que j'identifie que si je peux le faire avec peu d'effort. J'essaie de préciser le problème, selon moi, en me référant à la documentation existante dans la mesure du possible. Le fait de s'attendre à ce qu'un réviseur apporte une solution à chaque problème identifié crée un incitatif pervers - cela découragera le réviseur de signaler les problèmes.

BagOfSpanners
la source
3

Mon opinion est plus forte pour ne pas fournir de code dans la majorité des cas, pour un certain nombre de raisons:

  • Si l'explication ne suffit pas, ils peuvent toujours demander un échantillon de ce que vous pensez.
  • Vous ne perdez pas votre temps à essayer de vous familiariser avec un code que vous n'avez pas touché depuis longtemps, mais juste pour le modifier légèrement, alors que quelqu'un d'autre vient de passer tout son temps à le faire.
  • S'ils sont déjà familiarisés avec le code et que vous ne le connaissez pas déjà, donner uniquement les commentaires peut donner un meilleur code que celui que vous auriez écrit. Donner à quelqu'un une solution immédiate le poussera souvent à l'utiliser, sans envisager de l'améliorer davantage.
  • Toujours proposer une solution est proche de la condescendance. Vous travaillez avec quelqu'un, espérons-le, car ils ont eu la bonté d'être embauchés. Si vous avez réussi à apprendre pourquoi quelque chose est une mauvaise idée, pourquoi ne l'auraient-ils pas appris en écoutant les commentaires et en le faisant eux-mêmes?
  • Toujours fournir une solution est simplement étrange. Imaginez que vous programmez en binôme à leur bureau: ils viennent de terminer quelques lignes qui ne vous paraissent pas géniales. Dites-vous simplement ce que vous avez repéré et pourquoi, ou prenez-vous leur clavier et montrez-vous votre version immédiatement? C’est ce que vous pouvez toujours ressentir en offrant votre solution aux autres personnes.
  • Vous pouvez toujours dire ce que vous feriez à la place, sans perdre de temps à l'écrire. Vous venez de faire cela en décrivant le premier problème de la question.
  • Ne distribuez pas de nourriture, apprenez à pêcher;)

Bien sûr, il y a des cas où vous songez à une alternative spécifique, et il vaut la peine de la joindre. Mais c'est vraiment rare dans mon expérience. (beaucoup de critiques, de grands projets) L'auteur original peut toujours vous demander un échantillon s'il le souhaite.

Même dans ce cas, pour la troisième raison, il peut être utile de mentionner, par exemple, que "l’utilisation x.foo()rendrait ceci plus simple" plutôt qu’une solution complète - et laisse l’auteur l’écrire. Cela économise également votre temps.

viraptor
la source
Votre 5ème point m'a fait sourire, j'imaginais des "claviers en duel" pour voir qui pourrait obtenir une excellente solution en premier. Qui savait que la programmation en binôme pouvait ressembler à ces deux jeux d'arcade de course ou à un sport à contact complet? " Steve vient de vérifier brutalement Ron au BSOD, 2 minutes dans la surface de réparation ... "
2

Je pense que la clé de la révision du code est de s’entendre sur les règles avant la révision.

Si vous avez un ensemble de règles claires, il ne devrait pas être nécessaire de proposer une solution. Vous vérifiez simplement que les règles ont été suivies.

Le seul cas où la question d'un remplaçant se poserait serait si le développeur d'origine pensait à un moyen d'implémenter la fonctionnalité qui correspond aux règles. Supposons que vous ayez une exigence de performance, mais que la fonctionnalité prenne plus de temps que votre seuil après plusieurs tentatives d’optimisation.

Pourtant! si vos règles sont subjectives "Les noms doivent être approuvés par moi!" alors, oui, vous venez de vous nommer nommeur en chef et vous devriez vous attendre à recevoir des demandes de listes de noms acceptables.

Votre exemple d'héritage (paramètres facultatifs) est peut-être meilleur, car j'ai déjà vu des règles de révision de code interdisant les méthodes longues et les paramètres de fonction 'trop nombreux'. Mais normalement, ces problèmes peuvent être résolus de manière triviale en se scindant. Il s’agit de la partie "cette solution semble compliquer les choses de manière compliquée" lorsque votre objectivité est importune et nécessite peut-être une justification ou une solution alternative.

Ewan
la source
2
"Je pense que la clé des critiques de code est de se mettre d'accord sur les règles avant la révision." Ce serait le cas idéal. En pratique, nous ne pouvons pas supposer que tous les développeurs connaissent toutes les règles. Les revues de code sont utiles pour diffuser ces connaissances et expliquer les règles à l'aide d'exemples pratiques. C'est peut-être l'un des plus grands avantages de la révision de code.
Frank Puffer
notez les règles dans le document sur les normes de codage et remettez-les aux nouveaux développeurs
Ewan
1
Nous avons écrit des normes de codage qui sont données aux nouveaux développeurs. Cela fonctionne la plupart du temps, mais il y a parfois de mauvaises interprétations. Outre les normes de codage écrites, il existe des principes généraux tels que DRY ou SOLID qui sont également abordés dans les revues de code. Nous attendons de nos développeurs des connaissances de base à ce sujet et nous effectuons également quelques formations internes pour les améliorer. Ce processus est en cours et les révisions de code en font partie.
Frank Puffer
0

Si une solution potentielle est rapide et facile à dactylographier, j'essaie de l'inclure dans les commentaires de l'examen par les pairs. Sinon, j'énumère au moins mes préoccupations et les raisons pour lesquelles je trouve cet élément problématique. Dans le cas de noms de variable / fonction pour lesquels vous ne pouvez pas penser à quelque chose de mieux, je reconnais généralement que je n'en ai pas une meilleure idée et je termine le commentaire sous la forme d'une question ouverte au cas où quelqu'un pourrait . De cette façon, si personne ne propose une meilleure option, l'examen n'est pas vraiment retardé.

Pour l'exemple que vous avez donné dans votre question, avec la classe mal conçue. Je laisserais quelques commentaires selon lesquels, même s'il semble exagéré, l'héritage serait probablement le meilleur moyen de résoudre le problème que le code tente de résoudre, et de s'en tenir à cela. J'essayerais de formuler d'une manière qui indique que ce n'est pas une vitrine et qu'il appartient au développeur de décider s'il doit ou non réparer. Je voudrais également commencer par reconnaître que vous n'êtes pas très familier avec cette partie du code et inviter les développeurs et / ou les relecteurs plus avertis à préciser s'il y a une raison pour laquelle cela a été fait.

Eric Hydrick
la source
0

Allez parler à la personne dont vous révisez le code. Dites-leur, amicalement, que vous avez trouvé un peu difficile à comprendre, puis discutez avec eux de la manière dont cela pourrait être clarifié.

Les communications écrites entraînent de nombreuses pertes de temps, ainsi que du ressentiment et des incompréhensions.

Face à face, la bande passante est beaucoup plus élevée et l’on dispose du canal latéral émotionnel pour prévenir l’hostilité.

Si vous parlez réellement au gars, c'est beaucoup plus rapide et vous pourriez vous faire un nouvel ami et vous rendre compte que vous appréciez davantage votre travail.

John Lawrence Aspden
la source
cela ne semble rien offrir de substantiel sur les points évoqués et expliqués dans les 11 réponses précédentes
gnat