Que dites-vous dans une révision de code lorsque l'autre personne a créé une solution trop compliquée? [fermé]

L'autre jour, j'ai passé en revue le code écrit par quelqu'un de mon équipe. La solution n'était pas entièrement fonctionnelle et la conception était trop compliquée - ce qui signifie stocker des informations inutiles, créer des fonctionnalités inutiles et, fondamentalement, le code présentait une grande complexité inutile, comme le placage à l'or, et il tentait de résoudre des problèmes inexistants.

Dans cette situation, je demande "pourquoi cela a-t-il été fait de cette façon?"

La réponse est que l'autre personne avait envie de le faire de cette façon.

Ensuite, je demande si l'une ou l'autre de ces fonctionnalités faisait partie des spécifications du projet, si elles étaient utiles à l'utilisateur final, ou si l'une des données supplémentaires serait présentée à l'utilisateur final.

La réponse est non.

Je lui suggère donc de supprimer toute la complexité inutile. La réponse que je reçois habituellement est "bon, c'est déjà fait".

Mon point de vue est que ce n'est pas fait, que c'est bogué, que cela ne fait pas ce que les utilisateurs veulent et que les coûts de maintenance seront plus élevés que si c'était fait de la manière la plus simple que j'ai suggérée.

Un scénario équivalent est le suivant:
Colleague passe 8 heures à reformuler manuellement le code de refactoring, ce qui aurait pu être fait automatiquement dans Resharper en 10 secondes. Naturellement, je ne fais pas confiance au refactoring à la main car il est de qualité douteuse et n’a pas été complètement testé.
Encore une fois, la réponse que je reçois est "bon, c'est déjà fait".

Quelle est la réponse appropriée à cette attitude?

Mentalité / attitude

• Mener par l'exemple
• Admonish en privé (one-to-one, en dehors de l'examen du code)
• Encourager une mentalité Keep-it-simple parmi les membres de l'équipe

Gestion d'équipe

• Consacrez plus de temps à la spécification d'un élément de travail (architecture, aperçu de l'algorithme, structure filaire de l'interface utilisateur, etc.)
• Encouragez les membres de l'équipe à demander des éclaircissements sur la portée d'un élément de travail.
• Encouragez les membres de l'équipe à discuter des moyens de mettre en œuvre un élément de travail
• Faites des estimations raisonnables pour chaque élément de travail avant de commencer et faites de votre mieux pour les respecter.
• Surveiller "l'amélioration" des membres de l'équipe.
  • Après avoir été averti ou montré la bonne façon de faire les choses, voyez si le membre de l'équipe s'améliore.

Niveau de compétence

• Allouez du temps pour des sessions de programmation en binôme ou des sessions de formation individuelles afin de tirer le meilleur parti des outils de développement (refactoring, révision de code)

Gestion de projet (risque)

• Effectuer une révision du code plus souvent, de manière asynchrone (Remarque)
  • Note sur "asynchrone"
    • Le réviseur de code devrait recevoir des notifications / invitations à passer en revue les modifications dès leur validation.
    • Le réviseur de code devrait avoir la possibilité de réviser le code avant toute réunion avec le développeur.
    • Si des éclaircissements sont nécessaires de la part du développeur, faites-le de manière informelle sur messagerie instantanée / courrier électronique sans émettre d'avis négatif

Que dites-vous dans une révision de code lorsque l'autre personne a créé une solution trop compliquée?

Vous dites: "vous avez construit une solution trop compliquée."

Je lui suggère donc de supprimer toute la complexité inutile. La réponse que je reçois habituellement est "bon, c'est déjà fait".

S'il est trop tard pour changer quoi que ce soit, pourquoi effectuez-vous une révision du code?

"C'est déjà fait" n'est pas une réponse satisfaisante. Fait signifie testé et fonctionne. Tout code supplémentaire qui ne fait rien d’utile doit être conservé de la bonne manière (supprimé).

Affectez-lui à nouveau cette tâche en demandant à refactoriser et à optimiser sa solution. S'il ne le fait pas, assignez-lui un programmeur en binôme et espérez qu'il apprendra quelque chose du collègue.

Je lui suggère donc de supprimer toute la complexité inutile. La réponse que je reçois habituellement est "bon, c'est déjà fait".

Ce n'est pas une réponse acceptable:

• S'il est vraiment trop tard pour changer, alors la révision du code est en grande partie une perte de temps et la direction doit le savoir.

• Si c'est vraiment une façon de dire "je ne veux pas changer", alors vous devez prendre pour position que la complexité supplémentaire est mauvaise pour la base de code PARCE QUE le problème / le coût que cela va engendrer plus tard. Et la réduction du potentiel de problèmes futurs est la véritable raison pour laquelle vous effectuez la révision du code.

Et ...

... la solution n'était pas entièrement fonctionnelle ...

C’est peut-être une conséquence directe de la complexité inutile. Le programmeur a rendu le processus si complexe qu'il ne le comprend plus pleinement et / ou il a perdu son temps à mettre en œuvre sa complexité plutôt que les points de fonction. Il serait utile de faire remarquer au programmeur que le fait de réduire la complexité peut le faire accéder plus rapidement à un programme fonctionnel.

Maintenant, il semble que vous n’ayez pas le pouvoir (ou peut-être la confiance) de "repousser durement" à ce sujet. Mais même dans ce cas, cela vaut la peine de faire un peu de bruit à ce sujet (sans le personnaliser) dans l’espoir que le codeur incriminé fasse un meilleur travail ... la prochaine fois.

Quelle est la réponse appropriée à cette attitude?

En fin de compte, portez-le à l'attention de la direction ... à moins que vous n'ayez le pouvoir de le réparer vous-même. (Bien sûr, cela ne vous rendra pas populaire.)

Tu avais raison, ils avaient tort:

• principe de YAGNI brisé
• principe de KISS brisé
• le code est-il entièrement testé? Si non, alors ce n'est pas fait

Quelle est la réponse appropriée à cette attitude?

Faites la révision de code appropriée. S'ils refusent d'appliquer les modifications suggérées sans raison, arrêtez de perdre votre temps à revoir le code. Vous pouvez également transmettre le problème à leur patron .

Une des actions entreprises par notre équipe, qui a considérablement amélioré la situation dans de tels cas, a été le passage à des ensembles de modifications beaucoup plus petits .

Au lieu de travailler sur une tâche pendant au moins une journée et de procéder ensuite à une révision (de grande taille) du code, nous essayons de vérifier beaucoup plus souvent (jusqu'à 10 fois par jour). Bien sûr, cela présente également certains inconvénients, par exemple, le relecteur doit être très réactif, ce qui diminue son propre résultat (en raison des interruptions fréquentes).

L'avantage est que les problèmes sont détectés et peuvent être résolus rapidement, avant qu'une grande quantité de travail ne soit mal effectuée.

Vous devez vous concentrer sur la cause première du problème:

1. L'éducation des programmeurs se concentre sur la complexité croissante donnée aux programmeurs. La capacité de le faire a été testée par l’école. Ainsi, de nombreux programmeurs penseront que s’ils mettent en œuvre une solution simple, ils ne feront pas leur travail correctement.
2. Si le programmeur suit le même schéma qu'il a déjà pratiqué des centaines de fois à l'université, c'est exactement ce que pensent les programmeurs: plus la complexité est complexe , donc meilleure.
3. Donc, pour résoudre ce problème, vous devrez séparer de manière stricte les exigences de votre entreprise en termes de complexité par rapport à ce qui est normalement requis dans la formation des programmeurs. Un bon plan est une règle du type "le niveau de complexité le plus élevé ne doit être réservé qu'aux tâches conçues pour améliorer vos compétences - et il ne doit pas être utilisé dans le code de production".
4. Beaucoup de programmeurs seront surpris d'apprendre qu'ils ne sont pas autorisés à créer leurs conceptions les plus folles dans un environnement de code de production important. Prévoyez simplement du temps pour que les programmeurs puissent faire des conceptions expérimentales, puis gardez toute la complexité de ce côté de la clôture.

(dans l'examen du code, il est déjà trop tard pour le changer)

Je ne sais pas ce qui fonctionne après que le code a été écrit.

Avant que le code ne soit écrit, les gens peuvent discuter d'autres moyens de le faire. La clé est de partager des idées les unes avec les autres, donc nous espérons en choisir une raisonnable.

Une autre approche fonctionne avec les entrepreneurs: les contrats à prix fixe. Plus la solution est simple, plus le programmeur peut garder de l’argent.

Vous ne pouvez pas réparer le monde.

Vous ne pouvez même pas réparer tout le code de votre projet. Vous ne pouvez probablement pas corriger les pratiques de développement de votre projet, du moins pas ce mois-ci.

Malheureusement, ce que vous vivez dans la révision du code est bien trop courant. J'ai travaillé dans plusieurs organisations où je me trouvais souvent obligé de réviser 100 lignes de code qui auraient pu être écrites en dix, et j'ai obtenu la même réponse que vous: "c'est déjà écrit et testé" ou "nous cherchons bugs, pas une refonte. "

C'est un fait que certains de vos collègues ne peuvent pas programmer aussi bien que vous pouvez. Certains d'entre eux peuvent être assez mauvais à cela. Ne t'inquiète pas pour ça. Deux classes avec de mauvaises mises en œuvre ne feront pas baisser le projet. Au lieu de cela, concentrez-vous sur les parties de leur travail qui affecteront les autres. Les tests unitaires sont-ils adéquats (si vous en avez)? L'interface est-elle utilisable? Est-ce documenté?

Si l'interface avec le code incorrect est correcte, ne vous inquiétez pas jusqu'à ce que vous ayez à le maintenir, puis réécrivez-le. Si quelqu'un se plaint, appelez-le simplement refactoring. S'ils se plaignent encore, cherchez un poste dans une organisation plus sophistiquée.

Il doit exister une politique standard dans le projet qui contrôle les procédures et outils de contrôle de la qualité livrables.

Les gens doivent savoir ce qu’ils doivent faire et quels outils sont acceptés dans ce projet.

Si vous ne l'avez pas encore fait, organisez vos pensées et agissez.

La révision du code devrait avoir une liste de contrôle des éléments standard. Si vous obtenez "c'est déjà fait" et que ce n'est pas le cas, personnellement, je ne voudrais pas être responsable du travail de ce développeur en tant que chef de projet ou développeur principal. Cette attitude ne doit pas être tolérée. Je peux comprendre les arguments sur la manière de faire quelque chose ou même tout, mais une fois la solution acceptée, le mensonge ne devrait plus être toléré et cela devrait être clairement énoncé.

Votre boutique doit appliquer certaines méthodologies de conception.

• Les exigences doivent être clairement définies.
• Vous devez développer des cas d'utilisation qui prennent en charge les exigences.
• Vous devez spécifier les fonctions nécessaires pour implémenter les cas d'utilisation.
• Vous devez spécifier toute exigence non fonctionnelle (temps de réponse, disponibilité, etc.).
• Vous avez besoin d'un RTM (Requiements Tracabilty Matrix) pour mapper chaque fonction système à un cas d'utilisation et à une exigence réelle.
• Supprimez toute fonction qui ne prend pas en charge une exigence réelle.
• Enfin, dans votre révision de code, indiquez tout code qui n'implémente ou ne prend pas directement en charge les fonctions définies.

Probablement pas que ce soit trop compliqué car cela fait que la plupart des gens se sentent mal après. Je suppose que lorsque cela se produit déjà beaucoup de code a été écrit sans en dire un mot. (Pourquoi est-ce vrai? Parce que cette personne a suffisamment d'autorité pour que son code ne soit pas révisé dans la réalité?)

Sinon, je suppose que la révision du code est moins formelle mais plus fréquente. Et avant d'écrire de grands modules, vous devriez peut-être discuter rapidement de l'approche à adopter.

Dire "c'est trop compliqué" ne vous mène nulle part.

C'est malheureux, mais les revues de code le sont souvent plus pour l'avenir que pour le présent. Surtout dans un environnement d'entreprise / d'entreprise, le code livré a toujours plus de valeur que le code non expédié.

Cela dépend bien entendu du moment où la révision du code est terminée. Si cela fait partie du processus de développement, vous pourriez en tirer un bénéfice immédiat. Mais si le CR est traité plus comme un post-mortem, alors vous feriez mieux de préciser ce qui pourrait être amélioré à l'avenir. Dans votre cas (comme d'autres l'ont dit), indiquez YAGNI et KISS en général, et peut-être certains des domaines spécifiques où ces principes pourraient être appliqués.

Que signifie trop compliqué? Vous faites une déclaration ambiguë, puis vous obtiendrez une réponse ambiguë / insatisfaisante en réponse. Ce qui est trop compliqué pour une personne est parfait pour une autre.

Un examen a pour but de mettre en évidence des problèmes et des erreurs spécifiques, sans pour autant indiquer que vous ne l'aimez pas, comme le dit l'énoncé "trop ​​complexe".

Si vous voyez un problème (trop compliqué), dites quelque chose de plus concret, par exemple:

• Changer la partie X en Y ne simplifierait-il pas le code ou ne le rendrait-il pas plus facile à comprendre?
• Je ne comprends pas ce que vous faites ici dans la partie X, je pense que vous essayiez de faire ceci. Présenter une façon plus propre de le faire.
• Comment avez-vous testé cela? Avez-vous testé cela? Si c'est trop compliqué, cela conduira généralement à des regards vides. Demander des tests amène souvent la personne à simplifier elle-même son code lorsqu'elle ne sait pas comment tester le code d'origine.
• Il semble y avoir une erreur ici, changer le code pour résoudre ceci résoudrait le problème.

Tout le monde peut signaler des problèmes, en particulier des problèmes ambigus. Il existe un sous-ensemble beaucoup plus petit pouvant présenter des solutions. Vos commentaires doivent être aussi spécifiques que possible. Dire que quelque chose est trop complexe ne veut pas dire grand-chose, cela peut même amener les autres à penser que VOUS êtes incompétent pour ne pas être en mesure de comprendre le code. Gardez à l'esprit que la plupart des développeurs n'ont pas la moindre idée de la différence entre un bon et un mauvais design.

Parfois, il vaut la peine, en tant que groupe, de se concentrer sur certains des principes "Agiles" - ils peuvent aider un groupe ou une personne qui semble être légèrement en retrait.

Concentrer ne signifie pas forcément que votre équipe doit travailler beaucoup, mais vous devez tous vous asseoir et discuter des pratiques qui vous importent le plus. Je suggère de discuter au moins de ces (et probablement un peu plus):

• Est-ce que la chose la plus simple qui pourrait éventuellement fonctionner?
• Vous n'en aurez pas besoin (résolvez-vous des problèmes qui n'étaient pas dans les spécifications)
• Ecrivez le test avant de coder (vous aide à centrer votre code)
• Ne te répète pas

De plus, des critiques occasionnelles (hebdomadaires?) De ce qui fonctionne, de ce qui ne fonctionne pas et de ce qui est encore nécessaire peuvent être vraiment utiles ... Si rien d'autre, pourquoi ne pas vous engager à passer une heure par semaine pour discuter des valeurs et des pratiques de l'équipe?

Escalade, si vous avez un responsable technique. Cela ressemble à une habitude qui doit être brisée.

Si le code n'est pas construit selon les spécifications, alors, par définition, il devrait échouer à la révision du code. Je ne comprends pas le concept de "bien, nous avons fait quelque chose que personne n'a demandé, et cela ne fonctionne pas, nous allons donc le laisser là au lieu de faire quelque chose que quelqu'un a demandé qui fonctionne".

C’est une mauvaise habitude à prendre pour tout développeur. Si il / elle travaillait à une spécification de conception alors ne pas la faire correspondre sans bonne raison est un non non.

Un mot: agile

Cela ne résout certainement pas tout. Mais en contrôlant vos itérations (une à deux semaines, par exemple), en limitant les travaux en cours et en exploitant la planification / révision des sprints, vous devez éviter ces erreurs de type cascade. Vous avez besoin d'une meilleure visibilité sur ce qui est réellement fait - pendant que cela se fait.

Pour un développement normal basé sur un projet, je recommanderais d'adopter une approche Scrum . Pour les environnements de développement / d'intégration continus, et en particulier si plusieurs développeurs travaillent sur le même projet ou sur des projets associés, envisagez d'incorporer des éléments de Kanban . Une autre approche efficace consiste à exploiter la programmation en binôme , une pratique définie de la programmation extrême .

Votre situation est loin d’être unique. Et même avec de petites équipes, le processus peut aider à éviter la situation dans laquelle vous vous trouvez. Avec une visibilité adéquate et un arriéré raisonnablement soigné, des questions comme celles-ci deviennent des décisions de planification de sprint, vous évitant ainsi de gérer une dette technique .

Ce que j’ai dit par le passé, c’est "ce code est complexe et je ne suis pas sûr de ce qu’il essaie de faire. Est-il possible de le simplifier ou de l’écrire plus clairement?"

Vous devez coder après avoir supprimé / rétabli leur code: "Oups, votre code est parti. Veuillez le réécrire. Comme vous l'avez déjà écrit, il vous faudra moins de vingt minutes pour fournir SEULEMENT le code requis par la spécification.

"Mon prochain examen est dans 20 minutes.

"Bonne journée."

N'acceptez aucun argument!

Fait, à mon humble avis

Chris