Question:
L'architecte logiciel nouvellement promu souhaite effectuer des revues de code du code en cours
programmer
2016-11-13 04:34:26 UTC
view on stackexchange narkive permalink

Je travaille en tant qu'ingénieur logiciel avec une équipe de 12 personnes. Récemment, un développeur Sr a été promu architecte. Il a récemment eu une discussion avec le directeur du logiciel pour son manque de professionnalisme, son agressivité et sa dépréciation envers plusieurs membres de l'équipe. Ensuite, le directeur du logiciel nous a rencontrés individuellement pour essayer de nous faire travailler ensemble et essayer de le faire fonctionner.

Quelques jours plus tard, l'architecte s'est particulièrement intéressé à une fonction en cours (environ 3 jours) tâche) sur laquelle un autre ingénieur et moi-même sommes en train de programmer. Lors de notre réunion de mise à jour quotidienne du statut, il a convaincu mon chef de projet de lui permettre de faire une révision du code pour chaque commit quotidien du code en cours.

Nous utilisons notre système de contrôle de code source pour faire la révision du code sans avoir à tous être à une réunion en personne. Donc, mon partenaire et moi avons reçu la première critique avec plus de 20 commentaires sur des choses qui auraient été prises en charge s'il avait attendu de faire l'examen une fois la tâche terminée.

Personnellement, je n'ai jamais entendu parler de faire des révisions de code sur du code en cours. Cela me semble un gaspillage.

Ce qui me préoccupe, c'est qu'il pourrait essayer d'utiliser ces critiques comme fourrage pour saper mes compétences et causer des problèmes. Comment puis-je répondre professionnellement aux commentaires disant que mon partenaire et moi avons déjà planifié les choses supplémentaires qu'il souligne mais que nous les reportons à plus tard dans le processus?

MODIFIER : Pour clarifier et ajouter un peu plus de détails, les commentaires du réviseur de code ne concernaient pas l'ajout de tests ou de commentaires de code. Il s'agissait de choses comme l'ajout de vérifications d'autorisation aux points de terminaison d'API et l'ajout de configurations de préparation et de production. Nous les avons volontairement laissées de côté pour y accéder plus facilement pendant le développement. De plus, le changement des paramètres de configuration pendant le développement et la visite constante de l'étape et de la production pour chaque changement de paramètre de configuration est une perte de temps

La programmation en binôme n'est-elle pas la forme la plus extrême de révision en cours?Le problème avec la revue de votre architecte est qu'elle se fait sans le contexte de ce qui était prévu de faire aujourd'hui et de ce qui est prévu pour demain.
De quel genre de choses parlons-nous concernant «des choses qui auraient été réglées plus tard»?Si ses fonctionnalités, c'est juste, mais si ses commentaires, tests unitaires et autres choses similaires, alors votre architecte a un point - le code ne devrait pas être enregistré sans commentaires et tests correspondants déjà en place, ce ne sont pas le genre de chose sur laquelle vous revenez.Cette question ne peut pas être répondue sans beaucoup plus de détails.
Je pensais également que le code de programmation par paires n'avait pas besoin de révisions de code typiques car il y avait déjà un regard supplémentaire sur le code.Je pense que vous avez raison de dire qu'il avait besoin de ce contexte supplémentaire de ce qui était prévu.Cette boutique de développement est nouvelle pour faire des révisions de code (comme moi).Je ne sais tout simplement pas comment aborder cela.Je pense que devoir soumettre une révision de code chaque jour et fournir toutes nos «choses à faire» ajoute un travail chargé inutile qui n'ajoute pas de valeur.
@programmer non, vous faites toujours des révisions de code en programmation par paires - une partie du point de révision de code est que le réviseur est séparé du point du code engagé, donc il le passe en revue presque isolément, ne faisant pas partie d'une solution à un problème.De cette façon, le réviseur peut se concentrer sur la qualité du code plutôt que d'être aveuglé par la solution.
@Moo, Je teste le premier développement et pour ce commit particulier, nous avons eu des tests pour chaque méthode.Les commentaires du réviseur de code ne concernaient pas les tests ou les commentaires de code.Il s'agissait de choses comme l'ajout de contrôles d'autorisation aux points de terminaison d'API et l'ajout de configurations de préparation et de production.Nous les avons volontairement laissées de côté pour y accéder plus facilement pendant le développement.De plus, le changement des paramètres de configuration pendant le développement et la visite constante de l'étape et de la production pour chaque changement de paramètre de configuration est une perte de temps.
@programmer ajoute qu'à la question, les commentaires ont l'habitude de disparaître.La sécurité n'est pas non plus un travail «à faire plus tard», ni la configuration - pourquoi auriez-vous besoin de visiter un environnement, pourquoi n'avez-vous pas un système de déploiement automatisé et un système d'intégration continue?
@Moo, Je ne suis pas vraiment difficile de faire une révision de code sur le code programmé par paire.Je m'interroge simplement sur l'intérêt de faire des révisions de code sur du code en cours.Ce code est entièrement engagé dans une branche de développement, qui n'est pas prête pour la production ou même supposée être prête pour la production, mais cette revue était comme si elle aurait dû être prête pour la production.
@Moo, dites-vous que le code ne devrait pas être engagé dans le contrôle de code source tant qu'il n'est pas prêt pour la production?
@programmer Je dis qu'un commit qui a intentionnellement besoin d'être revisité n'est pas un commit que je ferais.Je travaille dans de petites fonctionnalités - chaque fonctionnalité principale est décomposée en blocs plus petits, chacun s'appuyant sur le dernier bit, chacun étant entièrement et 100% prêt pour la production.Si j'ai besoin de sauvegarder un travail en cours, il est caché et non validé.Selon ma façon de travailler, les révisions de code peuvent être effectuées à tout moment et ont du sens à tout moment.Le code incomplet ne doit être transmis à aucune succursale.
@Moo, Je n'ai pas le pouvoir de décomposer les tâches mais merci pour la perspective.Je pense que je vais commencer à faire exactement ce dont vous parlez concernant les commits.Que faire si l'architecte veut commencer à revoir notre code caché?Parce que c'est essentiellement ce qu'il veut faire.
@programmer Je ne pense pas qu'il le fera - mais s'il le fait, vous pouvez lui dire que le code est caché et donc pas destiné à être une version finale.S'il critique le code même à ce moment-là, trouvez un meilleur travail parce qu'il cherche simplement des problèmes.Les stashes sont destinés à être un bloc-notes qui peut être jeté à tout moment, les commits sont destinés à être l'histoire en devenir.
@programmer, il semble que vous vous êtes peut-être engagé trop souvent, ce qui semble bien au début mais rend le contrôle de version médiocre - un commit devrait être significatif.Une chose que vous voudrez peut-être envisager de faire si vous souhaitez toujours vous engager souvent est simplement de ne pas pousser la branche vers la télécommande - attendez que vous ayez une fonctionnalité ou une sous-fonctionnalité terminée, puis effectuez une validation de roll up ou de rebase, ce qui effacevos validations incrémentielles et vous donne une validation de fonctionnalité que vous pouvez ensuite pousser.
C'est fou.C'est comme dire que la viande n'est pas cuite lorsque vous la faites encore cuire.
@Moo, Haha, oui je m'engage plusieurs fois par jour.Un ancien collègue a mentionné la combinaison de commits.C'était une de ces choses que j'avais dans le fond de mon esprit pour commencer à faire mais je n'y suis pas encore arrivé.Je vais tourner une nouvelle feuille lundi avec le contrôle de source.:)
Dans mon entreprise, certaines personnes refusent catégoriquement les critiques de code «simples», il semble que votre entreprise se trouve de l'autre côté de l'excès.
@programmer, vous n'avez pas à décomposer les «tâches» pour décomposer le code en commits bitesize.Vous pouvez faire des commits testés, intégrés et en production qui n'ont pas encore d'effet sur le comportement du système et c'est une très bonne chose - bien mieux que d'intégrer une grande branche de fonctionnalités, de tirer le levier de déploiement en direct et d'espérer lebest (donc livraison continue).
@Moo: Vous pouvez vous engager dans votre propre agence aussi souvent que vous le souhaitez.Parce que votre propre branche est en cours de travail et que personne ne devrait se soucier de s'y engager.Dans ce cas, comme deux développeurs travaillent sur la même fonctionnalité, des commits fréquents dans leur branche partagée sont effectués pour la communication entre ces deux développeurs.Personne d'autre ne devrait s'en soucier.À la fin, la branche complète est revue - lorsqu'elle est terminée.
@gnasher729 Je ne suis pas d'accord, si vous poussez cette branche, vous déclarez que n'importe qui d'autre est libre de l'étudier à volonté.Si vous voulez garder la branche privée, ne la poussez pas vers le référentiel principal.Beaucoup de gens semblent utiliser le contrôle de code source comme un dépotoir pour le code dans n'importe quel état - c'est quelque chose qui ne devrait pas arriver, même lorsque plusieurs développeurs travaillent sur la même base de code.Fonctionnalités Push, pas de code.Si votre fonctionnalité est incomplète, ne la poussez pas vers le référentiel principal, cachez-la.
Pouvez-vous modifier votre question au lieu d'ajouter des commentaires ici?Il est vraiment difficile d'avoir une vue d'ensemble si la moitié de vos informations ne sont pas affichées par défaut.
@Moo Je ne suis pas du tout d'accord.Le code caché ne vous protégera pas contre les pannes du disque dur, pousser le fera.
@ Étienne a ensuite poussé de petites fonctionnalités complètes et autonomes - le problème des opérations est qu'ils validaient du code incomplet et étaient appelés dessus.Cela indique que les morceaux dans lesquels ils ont décomposé le travail sont trop monolithiques, ils doivent être davantage décomposés.Le fait qu'ils admettent dans ces commentaires que la configuration et l'autorisation faisaient partie des éléments manquants remis en question le confirme pour moi.
@Moo C'est votre opinion, mais je ne suis vraiment pas d'accord.Je pense qu'il est parfaitement normal de pousser le travail en cours vers une branche privée à la fin de la journée, même si ce n'est pas une compilation.Il s'agit d'un mécanisme de sauvegarde et n'a rien à voir avec l'apparence des commits dans la branche une fois le code prêt.Voir par exemple http://softwareengineering.stackexchange.com/questions/246328/how-can-a-manager-ensure-developers-are-pushing-up-to-the-origin-every-day
Huit réponses:
gnasher729
2016-11-13 12:58:19 UTC
view on stackexchange narkive permalink

Votre soi-disant "architecte" est ridicule. Examiner le travail en cours, à moins que la personne qui effectue ce travail ne le demande, est une perte de temps pour tout le monde. Ma réponse si quelqu'un voulait revoir mon travail en cours (et qu'il n'y a personne autour de moi qui fasse quelque chose de stupide comme ça) serait de lui dire "examinez tout ce que vous voulez, je m'en fiche car c'est un travail en cours, en attendant je avoir une conversation avec votre responsable sur le fait de perdre mon temps et votre temps ".

Les révisions de code sont attendues et appréciées après une pull request. Lorsque le code est complet. Pas à tout moment avant entre.

En fait, mon équipe lèvera souvent un PR avant d'être prêt et utilisera le système d'étiquetage de github pour ajouter la balise "WIP".Ceci est une * invitation * pour les revues de conception...et les revues de conception sont plus appréciées par l'OMI * avant * que tout le code ne soit écrit.Encore mieux, si vous avez le temps, c'est de rechercher et de présenter une conception de haut niveau avant de commencer à coder.Mais certains projets IME tombent dans "trop petit pour avoir une conception formelle à l'avance, mais suffisamment grand pour avoir un impact sur la conception du système d'une manière qui gagnerait à être révisée".La différence avec la situation d'OP est que cette situation est convenue d'un commun accord par projet.
Comme je l'ai dit, les révisions de code sont attendues et appréciées après une pull request.Dans ce cas, le critique a trouvé une énorme liste de «problèmes» dus au fait que le travail n'était pas encore terminé.Inutile et gaspillant le temps de tout le monde.
Oui, et si l'architecte examine des lignes de code, pas la conception / l'utilisation de la technologie, etc., alors ils ne regardent pas ce que je considérerais de toute façon comme un truc d'architecte.Je voulais juste souligner qu'il existe des motivations raisonnables pour examiner un code incomplet - et celles-ci sont compatibles avec le rôle d'un architecte logiciel.Trouver des fautes de frappe dans un code incomplet est une perte de temps.Trouver des problèmes majeurs avec le modèle de données, une répartition incorrecte des tâches entre les composants du système, etc., peut être mieux avec un retour d'information précoce car cela permet de gagner du temps.
Bien sûr, mais cela ne s'applique que si l'auteur du code cherche de l'aide.
En guise de suivi, informez votre directeur que l'architecte est en train de réviser le code du travail en cours et de perdre du temps à tout le monde.Vous pouvez mentionner que si les points qu'il a soulevés étaient valables, ils étaient également des points que vous connaissiez déjà et que vous prévoyiez de terminer au fur et à mesure de votre progression.Répétez que l'examen des travaux en cours pour les nitpicks est une perte de temps pour tout le monde.N'ouvrez aucune porte comme l'acceptation de points de révision architecturale, vous ne pensez pas que la révision du code avant la demande d'extraction ou quoi que ce soit de votre processus est utile.
Socrates
2016-11-13 09:22:45 UTC
view on stackexchange narkive permalink

On dirait que l'architecte a beaucoup trop de temps libre.

Quand les gens font des commentaires rédactionnels sans valeur sur mon code, je les ignore ou peut-être réponds par un superficiel: "Merci pour les bonnes idées! ", puis supprimez leur adresse e-mail.

Je vous suggère de faire de même. La vie est trop courte pour se soucier de choses qui ne sont pas productives comme ça.

Peut-être que l'architecte a également la tâche de maintenir la qualité du code élevée et que l'OP a déjà transformé un code médiocre.
@Robert ... et peut-être est-il un fouineur dont l'idée de progrès oblige tout le monde dans l'entreprise à utiliser des retraits à 2 espaces.
user42272
2016-11-13 13:27:44 UTC
view on stackexchange narkive permalink

Vraiment, il ne fait qu'une erreur, et je le laisserais faire cette erreur pour que vous puissiez vous concentrer sur une bonne relation avec lui dans une semaine ou deux.

"D'accord, je prévoyais sur la plupart de ceux-ci de toute façon, je vais donc jeter un autre coup d'œil dans un jour ou deux. À l'avenir, vous voudrez peut-être simplement faire un examen à la fin, mais si vous voulez y jeter un coup d'œil tôt, je suppose qu'il n'y a pas de mal. "

Je ne sais pas comment il réagirait à cela, mais je suis presque sûr que c'est une bonne idée pour le moment.

Burhan Khalid
2016-11-13 11:13:37 UTC
view on stackexchange narkive permalink

Vous avez deux problèmes comme je le vois:

  1. Votre superviseur ne comprend pas la programmation par paires; car les révisions de code font partie du fonctionnement de la programmation par paires.

  2. Il / elle n'est pas au courant du backlog, du sprint, ou de la manière dont vous gérez l'ensemble de fonctionnalités qui vous développez.

Un troisième problème possible pourrait être, il / elle fait cette évaluation par les pairs pour contrer les critiques antérieures de leurs supérieurs.

Dans tous les cas, ce n'est pas bon pour vous ou votre partenaire de programmation en binôme.

Je suggère de vous asseoir avec votre partenaire de programmation en binôme et cette personne, expliquez-leur comment vous prévoyez de développer la fonctionnalité, accueillir leurs révisions de code, mais expliquez quand elles sont les plus avantageuses pour vous . Cela pourrait être aussi simple que:

"Ce serait formidable pour nous si vous pouviez fournir les révisions de code pour les pulls d'hier le lendemain matin afin que nous puissions nous assurer que nous pouvons répondre à toute préoccupation immédiatement ce jour . "

Si vous faites attention, vous éviterez de passer pour quelqu'un qui ne prend pas bien les critiques (une plainte courante, en particulier dans le développement de logiciels); et en même temps, informez votre superviseur de la meilleure façon de vous aider.

Le simple fait est que revoir un code incomplet est un non-sens.C'est comme un mécanicien automobile qui sait qu'il doit faire les étapes a, b, c, d, e et f pour réparer vos freins, et après avoir fait a, b et c vous revoyez son travail et vous vous plaignez qu'il a fait un travail horrible parce que d, e et f sont manquants.
Thomas Cox
2016-11-14 05:53:01 UTC
view on stackexchange narkive permalink

La personne nouvellement promue souffre d'une erreur courante, "essayer d'ajouter trop de valeur". Il n'a pas vraiment les pieds sous lui. Il cherche des moyens d'être utile, mais son aide fait mal.

Voyez si vous parvenez à le confronter avec amour. "Mec, j'adore le fait que vous vous souciez de la qualité du code. Mais lorsque vous examinez du code qui est en train d'être programmé par paires, vous signalez les choses que nous allions réparer de toute façon. Cela vous fait perdre votre temps et votre talent. quelqu'un qui ne vous connaissait pas aussi bien que moi, cela pourrait nuire à votre relation avec eux. Puis-je vous offrir des conseils? "

(Attendez qu'il dise oui.)

"Vous avez été promu pour une raison. Soyez clair sur ce que vous pouvez faire uniquement pour nous dans votre nouveau rôle. Ce ne sont pas des révisions de code comme ça. C'est quelque chose. Quoi?

"La plupart des personnes nouvellement promues ont du mal à abandonner leurs anciennes activités. Ne te sens pas mal. Laissez tomber et concentrez-vous sur ce nouveau rôle.

"Je connais un architecte expérimenté, nommé __. Je serais ravi de vous présenter. Il m'a dit qu'il avait une tonne de valeur quand il était nouveau, en parlant avec des gens plus expérimentés. Je sais qu'il J'adorerais redonner. "

(D'accord, ne dites pas ça si vous ne savez pas __. ;-) Vous devrez peut-être faire un peu de devoirs.)

En résumé, il semble avoir du mal à trouver le meilleur moyen de remplir son nouveau rôle. Donnez-lui de la compassion et aidez-le (de manière appropriée).

colmde
2016-11-14 17:36:25 UTC
view on stackexchange narkive permalink

Pouvez-vous simplement répondre par un e-mail.

Bonjour [Architecte]

Merci pour vos commentaires. Pour votre information, ce travail est toujours en cours et il n'est donc pas encore prêt à être revu.

Nous vous ferons savoir quand il sera terminé

Merci, programmeur

et en rester là?

J'aurais aimé pouvoir mais il sait déjà que c'est du code en cours.Lors de la réunion de suivi quotidienne, il a dit qu'il savait que c'était en cours, mais qu'il pense qu'il serait utile de le revoir.
@programmer - Est-ce une option pour simplement semi-ignorer (c'est-à-dire ne pas ignorer, mais pas trop prioriser) ses commentaires et s'il se plaint juste gentiment lui dire quelque chose comme: "Ouais j'ai regardé vos commentaires - Comme je l'ai dit, le code est toujoursen cours, donc nous allions en faire une partie de toute façon. Ne vous inquiétez pas, nous allons vous faire part de vos commentaires ".
Xavier J
2016-11-13 20:28:05 UTC
view on stackexchange narkive permalink

Ce n'est pas un architecte. C'est un maniaque du contrôle. Il peut aussi bien dire à tous les développeurs de ne pas écrire de code à moins qu'il ne puisse s'associer avec eux sur une base personnelle.

Cette approche est complètement contre-productive. Envoyez ceci à votre patron.

Bien que cela soit vrai, ce n'est pas une réponse.
Robert
2016-11-16 09:40:56 UTC
view on stackexchange narkive permalink

Je voudrais aborder un point différent à ce sujet. Malheureusement, votre question ne contenait pas de détails, je ferai donc quelques hypothèses.

Premièrement, lorsque vous vous engagez dans le VCS, vous dites que vous avez atteint un stade de développement où le code engagé est utilisable par d'autres, surtout si vous vous engagez sur trunk / master. Une branche de fonctionnalité serait un peu différente, mais quand même: une fois que vous vous êtes engagé, vous pouvez vous attendre à ce que les gens y jettent un coup d'œil.

Il est beaucoup plus facile de revoir le code en petits morceaux. Même si tout n'est pas encore terminé, vous pouvez revoir ce qu'il y a et fournir des commentaires. Ceci est particulièrement important si le code semble aller dans la mauvaise direction ou s'il y a beaucoup de placage d'or.

En tant que chef d'équipe, j'ai eu du mal avec un développeur qui a toujours produit un code médiocre: manque de tests unitaires, Javadoc manquant, complexité inutile, code mort. Plus les morceaux de code que je devais examiner étaient volumineux, plus il était difficile de le comprendre, et plus il était difficile de suivre tous les commentaires que j'avais faits pour voir si le développeur les avait adressés. Malheureusement, vous ne dites rien sur le fait que vous et votre couple êtes junior ou senior, ou si vous et l'architecte avez déjà eu des désaccords sur la qualité du code.

Vous ne dites pas non plus quels commentaires vous recevez dans un révision du code. Tests manquants? Mauvaise dénomination? Code compliqué? Documentation manquante? Vous ne suivez pas la norme de codage de l'entreprise? Ce sont à mon humble avis tous les commentaires d'examen valides (peu importe si la fonctionnalité est encore terminée), et si vous ne les abordiez pas au fil du temps, je vous congédierais pour l'échec continu de l'exécution (après plusieurs avertissements, bien sûr).

Si vous aviez déjà prévu de faire les choses que l'architecte a indiquées, prenez cela comme un signe que vous êtes sur la bonne voie.

Enfin, ne nous précipitons pas aux conclusions: vous avez dit ceci était une tâche de 3 jours. Passer en revue trois jours de code fait à peine de l'architecte un maniaque du contrôle ou un micro-gestionnaire. Revenez si cela continue après 4 semaines et que vous n'avez pas été renvoyé (voir ci-dessus).

Vous pouvez toujours consulter tous les commits individuels même si vous attendez que toute la fonctionnalité soit terminée.Bien sûr, ce n'est généralement pas très utile car de nombreuses personnes le feront intentionnellement fonctionner de manière moche, puis passeront un peu de temps à corriger toutes les conventions de codage, de sorte que le produit final soit prêt pour la révision.Si OP est comme ça, c'est vraiment une perte de temps parce qu'il est conscient qu'il fait un gâchis;c'est parce qu'il n'a pas encore fini.
@Erik Désolé, la validation est une validation.Nettoyez avant de vous engager.
Je m'en tiendrai à "différentes opinions sur le moment de s'engager".


ⓘ
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 3.0 sous laquelle il est distribué.
Loading...