Quelle est la meilleure façon pour un programmeur de gérer la révision du code?

Je suis assez nouveau dans la révision de code, mais je code depuis des années pendant mon doctorat - ce qui ne fait pas toujours de vous un bon programmeur!

Si le réviseur modifie votre code et le passe en revue avec vous ligne par ligne, que faites-vous si vous n'êtes pas d'accord? Parfois, vous avez fait le choix X et le réviseur le change en Y, et vous aviez Y en tête, mais vous avez décidé de ne pas le faire en raison des inconvénients, mais le réviseur affirme que ces inconvénients ne sont pas importants. Devriez-vous verbaliser votre désaccord, ou tout simplement pas et les écouter?

C'est difficile si vous n'êtes pas bon pour accepter les critiques et si le réviseur est une personne plus âgée dans l'organisation. Il ne serait pas bon de se montrer trop défensif.

Certes, se présenter comme défensif n'est pas utile; mais alors - ni l'un ni l'autre n'est critiqué, donc si vous pensez que cela se produit, vous devriez vraiment exprimer vos préoccupations que la révision du code ne vous aide pas à écrire un meilleur code au sein de l'organisation.

L'évaluateur a la responsabilité d'examiner le code pour détecter toute non-conformité et défauts réels, et non de l'utiliser comme un moyen d'écrire votre code comme il l'aurait fait. Cela signifie que l'examen est là pour examiner comment vous avez fait quelque chose et signaler tous les domaines dans lesquels vous avez fait une erreur (ce que les meilleurs d'entre nous font) ou n'ont pas compris le cadre ou les normes ou les "raisons historiques". comme c'est là où tu es.

Donc, s'il y a plusieurs façons de faire quelque chose, il doit y avoir une bonne raison pour laquelle votre code est changé d'une manière alternative, sinon son désabonnement simplement non constructif qui ne vous aidera pas.

N'oubliez pas non plus que le critique n'est pas parfait non plus. Ils peuvent avoir une idée que Y est la façon de le faire, et n'ont pas réalisé que X est meilleur. Vous devez expliquer les raisons pour lesquelles vous l'avez fait de la manière X. Le réviseur pourrait être d'accord avec vous, ou il pourrait vous dire pourquoi Y est une meilleure solution - il peut y avoir d'autres facteurs que vous ne savez pas qu'il fait.

En bref, les révisions sont un moyen de faire communiquer les membres de l'équipe sur leurs modifications de code. Alors, parlez-en à l'examinateur.

SI cela vous aide, parlez de manière impartiale, comme si vous n'étiez même pas l'auteur du code en cours de révision. Le code appartient à l'entreprise ou à l'équipe après tout, et l'équipe devra le maintenir. Vous venez juste d'être le gars qui l'a écrit, ce n'est pas un facteur aussi important que beaucoup le croient.

L'écriture de code informatique est un excellent exemple de prise de décision dans l'incertitude . Il y a toujours des pressions contradictoires, et la façon dont vous décidez dans une question donnée dépend des pressions que vous percevez et de la taille que vous envisagez.

Par conséquent, lorsqu'un examinateur n'est pas d'accord avec votre décision, cela signifie qu'il voit des pressions / risques différents de vous, ou qu'il considère que certains d'entre eux sont plus grands / plus petits que vous. Vous devez absolument parler de ces différences, car ne pas le faire perpétue les problèmes qui ont conduit au désaccord en premier lieu.

Si votre examinateur est plus expérimenté, son expérience peut correctement lui dire que tel ou tel risque n'est pas très pertinent dans la pratique, ou il peut savoir qu'une sorte d'erreur a une longue histoire de se produire dans votre organisation, et cela vaut la peine d'être extrêmement prudent pour l'éviter. Inversement, si vous sentez que vous savez quelque chose que votre critique ne sait probablement pas, vous devez absolument l'exprimer; garder le silence équivaut à un manquement au devoir de votre part.

La partie la plus importante de la gestion de la situation est de comprendre que la critique des décisions de conception n'est pratiquement pas toujours une critique de la personnalité de quelqu'un. (Dans les rares cas où c'est effectivement le cas, vous remarquerez que assez tôt, et si vous ne pouvez vraiment pas plaire à quelqu'un, rien de ce que vous faites ne fait de différence, alors où est le mal de suivre les meilleures pratiques? dès que possible.) C'est simplement le résultat du fait que différentes personnes ont des perceptions différentes des nombreux risques et avantages inhérents au code informatique, et compte tenu de la complexité du code informatique moderne, il faut s'y attendre. Parler des différences permet d'améliorer le code et d' améliorer votre coopération dans ce cas et dans les cas futurs.

Les autres réponses contiennent déjà de très bonnes informations. Je voudrais développer un peu certains aspects auxquels a fait allusion gbjbaanb (voir mon commentaire sur sa réponse).

D'après mon expérience, j'ai observé différents types de commentaires lors des révisions de code:

1. "Je crois honnêtement que c'est faux / sous-optimal et que vous devriez le changer de cette façon." Je prends normalement ce genre de commentaires au sérieux et je ne m'opposerai au changement suggéré que si je pense avoir un point fort contre.
2. "Je trouve que votre solution est OK, considérez cette alternative mais c'est à vous de décider si vous acceptez ou non le changement." Je prends aussi ce type de feedback très au sérieux: le critique suggère une alternative, même s'il n'a pas une opinion forte que sa solution est meilleure. J'ai l'occasion d'apprendre quelque chose et j'accepterai le changement si je l'aime mieux. Sinon, le réviseur trouve OK si je garde le code selon mon goût.
3. "Je l'aurais fait différemment donc tu dois le changer, point final.", Ou même "Oh, j'ai changé ton code parce que ...", le changement n'a pas été suggéré, il a déjà été engagé! Une brève explication concernant le changement est donnée, avec l'indication qu'il n'y a pas beaucoup de temps pour discuter des détails car nous devons passer à la tâche suivante.
4. L'évaluateur suggère de petits changements de nature triviale qui n'améliorent pas le code mais le rendent simplement différent. Lorsqu'on lui a demandé de discuter des changements suggérés, le réviseur entame de longues discussions sur des détails triviaux jusqu'à ce que vous abandonniez par épuisement.

Les options 3, 4 peuvent être déguisées en 1 et 2: "C'était vraiment important de le faire comme je l'ai suggéré." ou "Vous pouvez refuser le changement si vous le voulez vraiment", a déclaré avec un ton qui signifie en fait tout le contraire de ce qui est dit. Dans le cas où vous vous opposez à des changements que vous jugez inutiles, la propriété partagée du code peut être utilisée pour justifier cette attitude: "Le code ne vous appartient pas, il appartient à l'équipe!" Vous pouvez savoir quand l'intention de l'examinateur n'est pas honnête si l'examinateur n'est pas très ouvert à la discussion, s'énerve et essaie de pousser sa solution à tout prix. Fondamentalement, ils ont déjà décidé, et toute autre discussion n'est qu'une perte de temps.

Les options 1 et 2 de l'OMI sont le signe d'un critique honnête qui essaie d'aider, essaie de vous enseigner quelque chose tout en respectant votre travail et est également ouvert à apprendre quelque chose lui-même. Dans ce scénario, j'essaie de ne pas être trop fier lorsque je reçois des commentaires constructifs d'un critique.

Les options 3, 4 suggèrent qu'il y a un jeu de pouvoir en cours: ce qui est important est de savoir si nous le faisons à ma façon ou à votre façon, pas que nous essayions de trouver une bonne solution qui satisfasse les deux. Cela peut être lié à l'ego de l'évaluateur, mais aussi à son incapacité à comprendre un code qui ne suit pas sa façon de penser. Ils sont normalement perturbés par du code qui ne leur semble pas familier et voudraient s'imposer à l'ensemble de la base de code.

Si les situations 3 et 4 se produisent trop souvent, le travail d'équipe peut devenir assez désagréable. Dans une telle situation, j'envisagerais de quitter l'équipe.

Pour répondre à votre question de savoir quoi faire lorsque vous n'êtes pas d'accord ...

En parler est notre voie à suivre, comme la plupart des gens l'ont déjà souligné.

Si vous l'avez déjà fait, peut-être même plus d'une fois, une technique utile que nous utilisons est s'il y a toujours un désaccord dans certains domaines, c'est de dire `` oui, il serait bon de refactoriser cela -

Pouvons-nous mettre un ticket séparé pour cela?

Un ticket séparé vous permet de saisir le code, au lieu du mode redouté de "churn and never move on" que je connais bien à certains endroits. Cela correspond bien à l'agilité, en faisant le moins possible et en répartissant la charge. Parfois, yagni finira par postuler. Parfois, le chef de produit décidera qu'il y a des besoins plus urgents que ce refactoriste en termes de valeur pour le client, de délais et de ressources.

La révision de code est probablement une bonne chose en général, mais d'après mon expérience, il est préférable de servir d'outil aux développeurs de nouvelles équipes en utilisant de nouvelles directives de codage ou pour corriger des bogues importants, de sorte que le risque de faire des erreurs soit réduit. Si vous pensez que vous savez mieux que le réviseur, vous devriez vous demander pourquoi la solution qu'il propose est meilleure et argumenter avec vos idées sur la question. Personne ne sait tout mieux, donc la révision du code devrait ou pourrait être une expérience d'apprentissage précieuse pour les deux.

La révision du code est à la fois une opportunité de détecter les problèmes potentiels et de transmettre les connaissances, à la fois pour le réviseur et le codeur.

En tant qu'examinateur de code, la responsabilité est de mettre en évidence les domaines de risque possibles, la non-conformité à la pratique standard, les améliorations et généralement juste un autre point de vue sur le même domaine de code.

Cela ne devrait pas entraîner un changement de code sans discuter / comprendre les décisions des codeurs au moment du développement.

Si le réviseur apporte des modifications, il peut avoir de la difficulté à déléguer du travail à d'autres, ce qui est difficile pour beaucoup de gens intelligents.

En tant que codeur recevant une évaluation, vous êtes protégé contre un problème possible lors de son déploiement, vous avez la possibilité d'apprendre quelque chose de nouveau et la possibilité de partager vos connaissances avec le réviseur.

Indépendamment de l'ancienneté, votre façon de penser peut proposer une solution qui ne vient tout simplement pas à l'esprit de certains, de sorte que l'examen peut être votre chance de briller simplement en faisant ce que vous pensez être juste.

Tant que le codeur et le réviseur acceptent qu'ils peuvent se tromper et que c'est correct de se tromper, une révision devient quelque chose qui renforce l'équipe parce que vous travaillez ensemble sur la solution.

Il semble que vous n'ayez pas encore fait réviser votre code :-)

Le but de l'examen du code est d'obtenir un code de qualité décente et de savoir que vous avez un code de qualité décente. Lorsque le code d'un développeur inexpérimenté est examiné, il peut être utilisé pour enseigner comment écrire un meilleur code, tout en évitant de frustrer ce développeur.

Le réviseur ne doit jamais modifier votre code. Ils peuvent faire des suggestions plus ou moins fortes sur la façon dont ils aimeraient que votre code soit modifié, et ils peuvent décider d'accepter ou non votre code.

Si l'examen se passe bien / si j'examine votre code, vous obtiendrez probablement des commentaires sur la façon dont je voudrais écrire le code que vous pouvez apprendre, ou ignorer - ce sont des choses où j'ai une opinion et vous êtes libre d'avoir un Opinion différente. Dans ma région, une bonne dénomination des fonctions, des variables, etc. est considérée comme importante, vous pouvez donc obtenir des suggestions pour améliorer la dénomination. Habituellement, vous devez apporter des modifications dans ce cas (parfois en trouvant un nom encore meilleur pour quelque chose). Parfois, je trouve des bugs. Vous les réparez. Parfois, je trouve des choses que je pense être des bugs, et je me trompe. S'il est difficile de voir que le code est correct, vous le rendez plus évident. Si je me trompe, dites-moi.

Si je pense que la conception n'est généralement pas correcte, cela aurait dû être discuté plus tôt. Nous devons ensuite nous demander si votre conception est assez bonne, en tenant compte de la quantité de travail qu'implique un changement, ou peut-être que je me trompais et que votre conception est meilleure. Nous devons nous retrouver avec un accord.

Si le réviseur et le révisé ne sont pas d'accord, nous avons un problème. Parce que cela signifie que l'un de nous est incapable de travailler en équipe, ou l'un de nous est incapable de faire la distinction entre une bonne ou une mauvaise conception, ou les deux. Ce n'est pas forcément de ta faute. Malheureusement, il y a des développeurs qui sont seniors et ignorants, et ce sera un problème pour l'entreprise et pour vous.

Si cela se produit, réfléchissez très, très fort: avez-vous un problème à accepter une critique fondée? Si tel est le cas, vous devez changer votre attitude. Êtes-vous trop inexpérimenté pour voir pourquoi l'examinateur a raison? Si c'est le cas, ce n'est pas un problème. Faites confiance à l'examinateur et apprenez. Etes-vous sûr que vous savez mieux que le critique? Acceptez l'avis, mais demandez à un troisième développeur de confiance son opinion. N'oubliez pas que vous pouvez être vraiment sûr de vous et avoir raison, mais vous pouvez également être vraiment sûr de vous et avoir tort.