Question:
Un collègue bloque la demande de changement dans l'examen par les pairs en raison d'erreurs perçues dans le code, mais les améliorations suggérées ne fonctionnent pas
Belle
2018-08-16 13:50:50 UTC
view on stackexchange narkive permalink

Dans l'entreprise pour laquelle je travaille, les demandes de changement passent par différentes étapes, dont une étape de développement, une étape d'examen par les pairs et une étape de test. Sur cette demande de modification particulière, j'ai été affecté en tant que développeur et un collègue développeur a été affecté en tant que réviseur.

Ce collègue est celui qui m'a formé à l'origine. Il travaille dans l'entreprise depuis plus de 10 ans, alors que je l'ai rejoint il y a un an, juste après l'université. Il a plus d'années d'expérience en tant que développeur que moi. Malgré cela, il est toujours mon pair car nous sommes tous les deux des développeurs de "niveau intermédiaire" (non plus juniors mais pas seniors).

Cette demande de changement particulière implique une fonction ancienne et compliquée. Il est mal écrit selon n'importe quel code standard, mais cela fonctionne. Il m'a fallu quelques heures pour trouver ce que j'avais besoin de changer. Le client voulait un ordre d'évaluation différent pour déterminer la valeur initiale d'un champ (par exemple, vérifiez client.address avant debtor.address au lieu de l'inverse).

Mon collègue a remarqué que je prenais du temps et j'ai regardé au code source avec moi. Il a indiqué un code et a dit: "Vous devez supprimer cela". J'avais des doutes à ce sujet, mais j'ai quand même essayé. L'essayer m'a fait comprendre où était le problème. En fin de compte, j'ai dû changer deux déclarations.

J'ai documenté le changement, l'ai testé et il a fonctionné comme prévu. J'ai transmis la demande de modification à mon responsable, qui a vérifié qu'elle fonctionnait et l'a transmise à l'évaluateur. Je l'ai reçu quelques minutes plus tard avec les commentaires "cela ne fonctionnera pas, vous avez changé le mauvais code" et "vous n'avez pas fait ce que j'ai dit".

J'ai dit que j'ai essayé ce qu'il dit, mais cela n'a pas fonctionné. Le code auquel il faisait référence n'était pas lié à la demande de changement, sauf qu'ils faisaient quelque chose avec ce même champ. Sa réponse a été que ce qu'il a dit n'était qu'un indice, parce que je dois être capable de comprendre les choses par moi-même.

Maintenant, il refuse de transmettre la demande de modification aux testeurs jusqu'à ce que je corrige mon erreur. Je lui ai demandé plus de détails, et il n'arrête pas de dire que je dois le découvrir par moi-même, car j'ai besoin d'apprendre. Je dois commencer par supprimer le code qu'il mentionne, selon lui. Puisque ce code n'a rien à voir avec la demande de changement en question, je suis très réticent. J'ai demandé à notre responsable, et il a dit de présenter mon argument au critique, car le responsable ne connaît pas la programmation. J'ai essayé cela, mais le critique est catégorique dans son argument et me rejette avec "fais ce que j'ai dit".

En partie à cause de l'âge de ce code, il n'y a pas de tests unitaires et ils ne peuvent pas être écrits pour eux sans réécrire la moitié de la base de code.

Comment résoudre cette situation?

Par curiosité, s'agit-il d'un département informatique d'entreprise ou d'un magasin de développement informatique?
Pouvez-vous transmettre la demande de modification aux testeurs sans son aide?
Faire réviser le code par des pairs est une perte de temps.Si le code fonctionne, alors tout ce qui ne va pas tombera dans la catégorie du jugement et de l'opinion, plutôt que dans celui des faits objectifs, ce qui en fait un travail de direction.
Neuf réponses:
#1
+192
berry120
2018-08-16 14:06:36 UTC
view on stackexchange narkive permalink

Il a peut-être raison de dire que le code doit être corrigé, mais il s'y prend de la mauvaise façon pour toute révision de code que j'ai jamais vue. Les révisions de code devraient échouer pour des points adressables très spécifiques, par exemple:

  1. Indentation incohérente dans ScoreHandler.cpp, lignes 45-50. Viole la ligne directrice de style n ° 8.
  2. getConsistentReview () dans Foo.java n'est pas thread-safe comme requis, la synchronisation sur le mauvais verrou.
  3. getFoo () dans Foo.java itère sur une liste pour filtrer les résultats. Puisque notre niveau source est maintenant de 8, cela devrait utiliser l'API de flux pour filtrer à la place, ce qui est beaucoup plus concis.

Il semble que vous ayez quelqu'un qui essaie (mal) de mentor vous en vous amenant à "réparer" le code comme il le souhaite, sans vous dire exactement pourquoi il est cassé ou ce qui ne va pas. On ne sait même pas si la raison pour laquelle il veut que vous le changiez est stylistique ou fonctionnelle.

Vous pouvez simplement refuser de jouer au ballon et le lui renvoyer. Que vous pensiez que cela fonctionnera globalement ou que cela causera plus de problèmes dépend de la culture.

Si vous voulez une approche plus douce, essayez de rédiger plus en détail ce que vous avez essayé, pourquoi cela n'a pas été le cas. t fonctionne, et indiquez exactement ce dont vous avez besoin pour continuer:

Bonjour x,

J'ai déjà essayé de supprimer le code y comme vous l'avez suggéré , mais cela provoque à la place z , ce qui entraîne l'échec de la fonction (ces) tests dans (ces) cas. Je ne suis pas très clair sur l'exigence ici - le changement que vous demandez est-il stylistique ou fonctionnel? Si c'est stylistique, pourriez-vous indiquer les règles de style spécifiques que le code enfreint, et si cela fonctionne, pourriez-vous me fournir un cas de test qui échoue?

Rien de ce qui précède n'est déraisonnable. demander dans une révision de code - il est plus déraisonnable que cela n'ait pas été fourni au départ.

S'il continue de dire "fais ce que j'ai essayé" après cela, vous pouvez simplement repousser avec "Comme ci-dessus, j'ai bien peur que cela n'ait pas fonctionné et j'aurai besoin de beaucoup plus de détails pour résoudre le problème problème." Si quelqu'un vous reproche d'avoir pris du temps, montrez-lui la trace écrite; il devrait devenir clair assez rapidement qu'il est celui qui traîne les pieds et refuse de jouer au ballon.

Mise à jour: juste essayé et cela a fonctionné.Il a fini par venir à mon bureau et m'a aidé à améliorer mon code.Ce n'était pas mal après tout, mais cela pourrait être mieux.Sa solution n'était pas complète, mais n'était pas aussi mauvaise que je le pensais.Je suppose que nous avions tous les deux tort.
@Belle-Sophie Peut-être a-t-il vu ce post et s'est-il rendu compte que vous êtes vraiment coincé.
@Belle-Sophie Excellent à entendre, heureux de vous aider.
@Belle-Sophie: Je pense que c'est une bonne leçon pour l'avenir;oui les revues de code sont généralement menées de manière asynchrone via des outils ... cependant chaque fois que vous vous trouvez confus par une question / réponse dans la revue de code, il peut être préférable de passer à la communication téléphonique / face à face pour dissiper rapidement les malentendus:)
Pourtant, y avait-il un avantage à ses changements?Ou était-ce juste une perte de temps pour tout le monde?
Une autre possibilité est que le réviseur ait vu des cas d'utilisation supplémentaires qui n'ont pas été corrigés.Il peut s'agir ou non de bogues supplémentaires, selon les détails du rapport de bogue et la façon dont les bogues sont gérés.Quoi qu'il en soit, l'approche fonctionne toujours.
@Belle-Sophie:, veuillez mettre à jour votre question avec cela, car cela invalide à peu près toute la prémisse de votre question
OPL "Le code auquel il faisait référence n'était pas lié à la demande de modification, ** sauf qu'ils faisaient quelque chose avec ce même champ **."[c'est moi qui souligne].Donc c'est * lié *!
#2
+18
gnasher729
2018-08-16 14:05:20 UTC
view on stackexchange narkive permalink

Dans le flux de travail de mon entreprise, ce serait facile.

Marquez la demande de modification comme "ne sera pas corrigée" avec un commentaire "correctif correct rejeté par l'évaluateur", puis attribuez-lui la demande de modification. De toute évidence, il sait comment y remédier, donc cela devrait être un travail de cinq minutes pour lui.

Et en fait, s'il avait raison, alors ce serait la bonne chose à faire. Inutile de perdre votre temps. Il n'y a donc rien dont il puisse se plaindre.

Avec un code ancien et obscur, deux choses peuvent arriver: je le laisse inchangé, ou je l'améliore et j'assume la responsabilité s'il casse. La seule chose qui ne se produira pas , c'est que j'ordonne à quelqu'un d'autre de faire ces changements. Et certainement pas lors d'une révision de code.

J'ai essayé ça.Il me la renvoie juste.
C'est là que vous l'amenez à votre responsable.Votre responsable ne connaît pas la programmation, mais vous dites que les suggestions de vos collègues ne fonctionnent pas, il dit qu'elles fonctionnent, donc dans cette situation, il serait évident pour votre patron qu'il devrait faire le travail.
@Belle-Sophie Alors ... il insiste pour que vous fassiez _ son_ changement sous _ votre_ nom?Ce n'est pas très cricket.Je suggère d'ajouter ce détail à votre question car il met en évidence la mentalité du gars
@rath Pouvez-vous expliquer votre utilisation du mot «cricket» dans ce contexte?J'ai peur d'être complètement perdu.
@Two-BitAlchemist est un terme anglais pour signifier "tout simplement pas la bonne façon de faire les choses avec honneur", des parties de cricket où les joueurs étaient toujours censés jouer avec équité et conduite gentleman.
Yeesh.Vous savez ce qui n'est pas "cricket"?Répondre à douchebaggery avec plus de douchebaggery.Les commentaires passifs-agressifs dans le code (et les révisions de code) sont un énorme drapeau rouge sur l'aptitude d'une personne à collaborer.Ne fais pas ça.
"Won't fix" est généralement une raison proche sur un ticket.Pourquoi fermeriez-vous le ticket avant de le réattribuer?
Donc, si vous transmettez des problèmes, vous ne savez pas comment faire;comment apprendrez-vous à les faire?
#3
+6
rath
2018-08-16 14:34:21 UTC
view on stackexchange narkive permalink

Cela peut ne pas s'appliquer à vous pour des raisons techniques ou organisationnelles, mais la réponse générale est:

Faites un test unitaire

Un test unitaire est la preuve qu'un bogue a été corrigé. Chaque fois que vous corrigez un bogue, ou chaque fois que vous avez besoin de démontrer un bogue, les tests unitaires économisent de nombreux arguments. Voyez s'il y a une personne chargée du contrôle qualité à qui vous pouvez parler et régler celle-ci.

N'oubliez pas qu'il n'a pas rejeté votre modification parce qu'elle n'a pas fonctionné, il l'a rejetée parce que ce n'était pas son changement. Avoir un test unitaire de sauvegarde renforce votre position.


Spécifiquement pour votre situation: rappelez-vous que s'il veut que vous fassiez ce qu'il a dit, il devrait le faire lui-même. C'est votre tâche et vous pouvez faire ce que vous dites. C'est votre mentalité, maintenant pour le dialogue.

Appelez le développeur et parlez avec lui. Démontrez que votre correctif fonctionne, et si on vous demande pourquoi vous n'avez pas fait ce qu'il a dit, vous pouvez dire quelque chose comme ceci:

Ce n'est pas exactement ce que vous avez dit mais cela fonctionne et corrige le problème. Je comprends cette solution dans ma tête, l'inverse fonctionnerait probablement aussi mais m'aurait pris beaucoup plus de temps à comprendre. Je pense donc que nous pouvons marquer celui-ci comme Réussi dans la révision du code?

J'aime cette réponse.Malheureusement, cela ne fonctionnera pas pour moi (j'ai mis à jour la question), mais c'est une bonne réponse pour les personnes ayant un problème similaire qui est en fait testable.
@Belle-Sophie Je pensais que votre situation pourrait être un peu différente :) Le gars ressemble à un collègue que je n'aimerais pas avoir
On dirait que vous avez une résolution, mais pour la personne suivante dans cette situation - bien qu'il soit généralement peu pratique de tester entièrement une fonction héritée volumineuse, il est presque toujours possible de factoriser chaque élément de logique individuel en une fonction testable, puis de les enchaîner.ensemble, et souvent le fait de faire cela fait naître une idée là où se trouve l'erreur.
#4
+3
DigitalBlade969
2018-08-16 15:03:30 UTC
view on stackexchange narkive permalink

Je suis d'accord avec berry120. Cependant, vos notes d'examen étaient

"cela ne fonctionnera pas, vous avez changé le mauvais code" et "vous n'avez pas fait ce que j'ai dit"

Ma suggestion:

(Ne passez pas plus d'une demi-journée à ce sujet. Documentez de manière concise le comportement du code pertinent, les erreurs / avertissements, vos actions entreprises et les raisons de le faire so.)

  • assurez-vous que votre nouveau code fonctionne dans la version X (pas d'erreurs ni d'avertissements) selon la demande du client
  • créer un fork ou une version le code avant vos modifications
  • fait exactement comme l'état des notes (essayez de corriger les erreurs le cas échéant), c'est maintenant la version Y
  • si ces modifications ne répondent pas à la demande du client notez ceci en particulier et essayez d'inclure la version X dans la version Y
  • informez (par e-mail) votre réviseur et responsable:

Je crois avoir terminé la mise en œuvre le client a demandé des modifications dans la version X.
Pour répondre aux notes de révision, j'ai suivi les étapes supplémentaires suivantes pour la version Y.

Listez ici vos progrès et résultats, incluez votre documentation sur ce processus (soyez bref, 5 à 10 lignes max.).
Si cela facilite la lisibilité, vous pouvez également avoir cette liste à la fin du e-mail et reportez-vous-y en conséquence.

Si les notes de révision n'ont pas répondu à la demande du client:
(ce qui suit expliquera votre retard et votre non-respect initial des notes)

Pour autant que je sache (voir mon rapport ci-dessus / ci-dessous), les notes d'examen à elles seules ne répondraient pas à la demande des clients et j'ai également implémenté la version X.

J'ai d'abord écrit la version X pour m'assurer que la demande des clients était traitée avant d'ajouter d'autres modifications de code.

Si la version Y contient encore des erreurs que vous ne pouvez pas corriger:
(Ce sera probablement les notes de révision ou l'ajout de la version X aux notes de révision - ajustez la formulation ci-dessous en conséquence!)

Je suis heureux de continuer à travailler sur la version Y.
Malheureusement, l'intégration des notes de révision avec la version X (le client de travail a demandé des modifications) s'avère plus complexe que la soumission de la version X seule et j'ai besoin de plus de temps.

Je suis sur la clôture d'inclure ce qui suit pour apaiser un peu votre réviseur tout en signalant à la direction que ses modifications sont à l'origine des erreurs. Cela peut également amener la direction à interpréter que vous avez toujours besoin de la supervision des réviseurs.

insérer le nom du réviseur , si vous avez un moment, j'apprécierais grandement vos conseils concernant la motivation derrière (ou peut-être moins curieux: "la nature de ") votre code change et les erreurs qui en résultent pour la version Y.

Utilisez évidemment les numéros de version réels au lieu de X / Y (;

MODIFIER:

Pour rendre ma réponse plus lisible et plus concise, je l'ai modifiée. Veuillez laisser des commentaires si c'est mieux. merci.

#5
+3
ventsyv
2018-08-17 21:00:22 UTC
view on stackexchange narkive permalink

Quelque chose de similaire m'est arrivé - mon premier emploi après l'université, un développeur senior a dû faire un changement pour prendre en charge quelque chose sur lequel je travaillais, mais le changement qu'il a fait n'a pas fonctionné. Ce qui a fonctionné pour moi, et ce que je vous suggère de faire, c'est ce qui suit:

Impliquez un autre développeur senior, organisez une réunion avec les deux et expliquez votre point de vue. Si possible, démontrez les tests que vous avez exécutés. Puis laissez-le expliquer son approche. Déterminez quelle est la meilleure solution et faites-le.

Assurez-vous de ne pas apparaître comme un adversaire, quelque chose du genre:

Salut Mike. Il semble que nous continuons à nous parler du bug # 123. Avez-vous 30 minutes le lundi pour en parler? Je vais demander à Jim de nous rejoindre depuis qu'il y travaille récemment

#6
+2
Dan
2018-08-17 22:28:54 UTC
view on stackexchange narkive permalink

Lors de mon dernier emploi, nous avions un processus de type examen par les pairs. La direction nous a dit de ne pas dire à la personne comment le résoudre, mais seulement de lui donner des conseils et des conseils. En tant que tel, j'ai été à l'opposé de la situation OP où je suis le développeur senior en train de former une nouvelle personne qui a "résolu" un problème mais les changements ne tiennent pas compte de certaines situations uniques.

La façon dont je l'ai résolu expliquait directement les situations uniques et la personne était capable de coder autour de ce comportement, mais le code global était sujet aux erreurs car il s'agissait d'un ancien dépôt. Je pense que vous devriez demander au développeur s'il existe une sorte de situation unique que vous ne réalisez pas qu'il peut prévoir. S'il n'y a pas de cas unique et qu'il dit que c'est "cassé" parce que vous ne l'avez pas suivi, remettez-le lui. Écrivez dans le ticket le code fonctionne, il a été testé par la direction, et malheureusement vous ne voyez aucune situation où le code serait erroné.

#7
  0
gbjbaanb
2018-08-16 19:39:28 UTC
view on stackexchange narkive permalink

Il semble étrange qu'il refuse votre solution et refuse de vous donner des détails. Il est donc temps de dégénérer un peu. Cela signifie faire apparaître l'e-mail qu'il vous a envoyé à votre chef d'équipe disant que vous ne comprenez pas pourquoi votre correctif a été rejeté et que vous ne comprenez pas suffisamment la fonctionnalité pour comprendre ce que votre collègue dit, vous demandez donc l'aide du TL.

J'exprimerais cela en termes "d'anciennes fonctionnalités dont vous ne comprenez pas l'historique ou les implications de", mais vous voudrez peut-être affiner cela, mais vous vous adresserez au TL "pour obtenir de l'aide cette tâche "sachant qu'il verra la trace des changements de code (ie" montrez-moi ce que vous avez essayé jusqu'à présent ") qui mettra en évidence le manque de coopération de votre collègue. Cela devrait suffire à donner à cette tâche une certaine visibilité dans l'équipe et à se concentrer sur sa résolution, sans obstacles artificiels et inutiles de la part de votre collègue (qui agit de manière non professionnelle).

À aucun moment, vous n'avez besoin de se plaindre, ou même mentionner l'inutilité du collègue, ce sera clair.

Si vous n'avez pas de chef d'équipe, remettez-le dans la file d'attente "todo" et mentionnez (lors d'une réunion de progression?) que vous ne comprenez pas suffisamment le problème pour le résoudre après quoi vous avez déjà essayé, vous laissez donc le soin à quelqu'un de plus expérimenté de le réparer.

#8
  0
thelem
2018-08-20 16:20:42 UTC
view on stackexchange narkive permalink

Votre collègue vous a donné quelques indices et a déclaré que vous deviez apprendre. Ils essaient d'être utiles et de vous donner les outils pour résoudre vous-même des problèmes similaires à l'avenir.

Vous avez essayé leurs astuces, mais pour une raison inconnue, elles n'ont pas fonctionné (soit les astuces avaient tort, vous ne les compreniez pas, ou une combinaison des deux). Vous ne pouvez pas résoudre l'un ou l'autre de ces problèmes par vous-même, vous devrez donc demander de l'aide au réviseur. Lorsque vous faites cela, vous devez expliquer au réviseur ce que vous avez essayé et pourquoi cela n'a pas fonctionné.

Cela démontre que vous faites des efforts vous-même, plutôt que de vous fier à leur expérience pour résoudre le problème (ce qui pourrait faire de vous un "vampire d'aide"). Si leurs suggestions étaient fausses, ils se rendront compte à ce stade et vous pourrez alors discuter d'autres solutions. Si vous aviez mal compris leurs suggestions, ils comprendront mieux votre façon de penser et pourront reformuler leurs suggestions pour vous éloigner de votre malentendu.

#9
  0
UKMonkey
2018-08-20 20:18:03 UTC
view on stackexchange narkive permalink

En lisant les commentaires, la réponse que vous semblez réellement utiliser, c'est de parler à la personne de vos difficultés et de ses attentes; puis déterminez pourquoi ce qu'ils s'attendaient à fonctionner n'a pas fonctionné.

Quels que soient les outils utilisés par votre entreprise pour suivre les problèmes, ce sont généralement des outils de tenue de registres et de communication; et bien qu'ils aient leur place, je pense que cela démontre que la communication verbale ne peut être remplacée.

Pour être honnête, cela devrait être le premier port d'escale pour de nombreux problèmes liés au travail; car cela fait simplement partie de la façon de travailler en équipe.



Ce Q&R a été automatiquement traduit de la langue anglaise.Le contenu original est disponible sur stackexchange, que nous remercions pour la licence cc by-sa 4.0 sous laquelle il est distribué.
Loading...