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

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?

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.