Passez en revue avant ou après la validation du code, quel est le meilleur?

Traditionnellement, nous examinions le code avant le commit, je me disputais aujourd'hui avec mon collègue, qui préférait le revoir après le commit.

D'abord, voici un peu de contexte,

1. Nous avons des développeurs expérimentés et nous avons également de nouvelles recrues avec une expérience en programmation quasi nulle.
2. Nous aimerions effectuer des itérations rapides et courtes pour libérer notre produit.
3. Tous les membres de l'équipe sont situés sur le même site.

Les avantages de la révision du code avant la validation sont les suivants:

1. Mentor nouvelles embauches
2. Essayez de prévenir les erreurs, les échecs et les mauvaises conceptions au début du cycle de développement
3. Apprendre des autres
4. Sauvegarde des connaissances si quelqu'un quitte

Mais j'ai aussi eu de mauvaises expériences:

1. Faible efficacité, certains changements peuvent être examinés au fil des jours
2. Difficile d'équilibrer vitesse et qualité, surtout pour les débutants
3. Un membre de l'équipe s'est senti méfiant

Pour ce qui est de l'examen post-engagement, je sais peu de choses à ce sujet, mais ce qui m'inquiète le plus, c'est le risque de perdre le contrôle en raison d'un manque d'examen. Des opinions?

MISE À JOUR:

1. Nous utilisons Perforce pour VCS
2. Nous codons et commettons dans les mêmes branches (branches de correction de tronc ou de bogue)
3. Pour améliorer l'efficacité, nous avons essayé de scinder le code en petites modifications. Nous avons également essayé d’examiner certains dialogues en direct, mais tout le monde n’a pas suivi la règle. Ceci est un autre problème cependant.

Comme Simon Whitehead le mentionne dans son commentaire , cela dépend de votre stratégie de branchement.

Si les développeurs ont leur propre branche privée pour le développement (ce que je recommanderais dans la plupart des cas), j'examinerais le code avant de le fusionner avec le référentiel principal ou principal. Cela permettra aux développeurs de disposer de la liberté d’archiver aussi souvent qu’ils le souhaitent au cours de leur cycle de développement / test, mais tout code temporel envoyé dans la branche contenant le code livré est examiné.

En règle générale, vos mauvaises expériences avec les révisions de code ressemblent davantage à un problème lié au processus de révision qui apporte des solutions. En examinant le code en petits morceaux individuels, vous pouvez vous assurer que cela ne prend pas trop de temps. Un bon nombre est qu’il est possible de réviser 150 lignes de code en une heure, mais le débit sera plus lent pour les utilisateurs non familiarisés avec le langage de programmation, le système en cours de développement ou la criticité du système (un problème de sécurité nécessite plus de temps) - Ces informations peuvent être utiles pour améliorer l'efficacité et déterminer qui participe aux revues de code.

Il y a un mantra que personne ne semble avoir encore cité: Arrivez tôt et souvent :

Les développeurs qui travaillent pendant de longues périodes - et de loin, je veux dire plus d'une journée - sans rien vérifier dans le contrôle de source se préparent à de graves problèmes d'intégration par la suite. Damon Poole est d'accord :

Les développeurs retardent souvent l'enregistrement. Ils le font parce qu'ils ne veulent pas affecter les autres personnes trop tôt et qu'ils ne veulent pas se faire reprocher de briser la construction. Mais cela pose d'autres problèmes, tels que la perte de travail ou l'impossibilité de revenir aux versions précédentes.

Ma règle de base est "check-in tôt et souvent", mais avec l'avertissement que vous avez accès à la gestion des versions privée. Si un enregistrement est immédiatement visible par les autres utilisateurs, vous courez le risque d'introduire des modifications immatures et / ou de casser la construction.

Je préférerais de loin que de petits fragments soient enregistrés périodiquement plutôt que de passer de longues périodes sans avoir la moindre idée de ce que mes collègues écrivent. En ce qui me concerne, si le code n'est pas archivé dans le contrôle de source, il n'existe pas . Je suppose que c'est encore une autre forme de Don't Go Dark ; le code est invisible jusqu'à ce qu'il existe dans le référentiel sous une forme ou une autre.

... Si vous apprenez à vous enregistrer tôt et régulièrement, vous aurez amplement le temps de faire vos commentaires, de s'intégrer et de passer en revue les autres ...

J'ai travaillé pour quelques sociétés ayant des approches différentes à cet égard. On l'a permis, tant que cela n'empêchait pas la compilation. L'autre paniquerait si vous enregistriez des bugs. Le premier est de loin préféré. Vous devriez vous développer dans une sorte d’environnement qui vous permettrait de collaborer avec d’autres personnes sur des projets en cours, tout en sachant que tout sera mis à l’essai ultérieurement.

Il y a aussi la déclaration de Jeff Atwood: N'ayez pas peur de casser des choses :

Le moyen le plus direct de s’améliorer en tant que développeur de logiciels est de ne pas avoir peur de changer de code. Les développeurs qui ont peur du code erroné sont des développeurs qui ne deviendront jamais des professionnels.

J'ajouterais également que pour les examens par les pairs, si quelqu'un souhaite que vous changiez quelque chose, le fait d'avoir l'historique de votre version originale à la source est un outil d'apprentissage très précieux.

J'ai récemment commencé à faire des revues de pré-engagement dans un projet dans lequel je suis et je dois dire que je suis agréablement surpris de voir à quel point c'est sans problème.

Le plus gros inconvénient des critiques post-commit que je vois, c’est que c’est souvent une affaire impliquant une seule personne: Quelqu'un révise le code pour s’assurer de son exactitude, mais l’ auteur n’est généralement pas impliqué sauf s’il ya une grave erreur. Les petits problèmes, suggestions ou astuces ne parviennent généralement pas du tout à l'auteur.

Cela modifie également le résultat perçu des révisions de code: il est perçu comme un élément ne générant qu'un travail supplémentaire, par opposition à un élément permettant à tout le monde (le réviseur et l'auteur du code) d'apprendre chaque fois de nouvelles choses.

Les révisions de code pré-engagement semblent si naturelles, il ne m'est jamais venu à l'esprit que des révisions pourraient être délibérément effectuées après avoir été validées. Dans une perspective d'intégration continue, vous souhaitez engager votre code une fois qu'il est terminé, et non dans un état de travail mal écrit, mais non?

C'est peut-être parce que mes équipes ont toujours procédé de la sorte: le dialogue en direct a été lancé par le développeur d'origine, et non des révisions asynchrones, basées sur des contrôles et basées sur des documents.

Aujourd'hui, la plupart des référentiels prennent en charge un commit en deux phases ou un plateau (branche privée, demande d'extraction, soumission de correctif ou quoi que vous souhaitiez l'appeler), ce qui vous permettra d'inspecter / de réviser le travail avant de l'insérer dans la ligne principale. Je dirais que tirer parti de ces outils vous permettrait de toujours effectuer des examens préalables à l’engagement.

Vous pouvez également envisager le codage par paire (paires senior avec junior) comme un autre moyen de fournir une révision de code intégrée. Considérez-le comme une inspection de la qualité sur la chaîne de montage plutôt que lorsque la voiture s’est retirée.

Faire les deux :

• pre commit - fait ce genre de critiques quand c'est quelque chose de très important, comme un code très réutilisable ou une décision de conception importante
• post commit - faites ce genre de commentaires lorsque vous souhaitez obtenir un avis sur un élément de code susceptible d'être amélioré

Toute révision formelle doit être effectuée sur les fichiers source sous contrôle de configuration, et les enregistrements de révision indiquent clairement la révision du fichier.

Cela évite les arguments de type "vous n'avez pas le dernier fichier" et garantit que tout le monde examine la même copie du code source.

Cela signifie également que, si des corrections postérieures à la révision sont nécessaires, l'historique peut être annoté avec ce fait.

Pour la révision du code lui-même, mon vote est pour 'pendant' le commit.

Un système comme gerrit ou trèfle (je pense) peut mettre en place un changement puis demander à l'examinateur de le valider dans le contrôle de source (push in git) s'il est bon. C'est le meilleur des deux mondes.

Si cela n’est pas pratique, je pense qu’après l’engagement, c’est le meilleur compromis. Si la conception est bonne, seuls les développeurs les plus juniors devraient avoir des problèmes suffisants pour que vous ne les vouliez plus jamais. (Faites une revue préalable à leur engagement pour eux).

Ce qui conduit à la révision de la conception - bien que vous puissiez le faire au moment de la révision du code (ou au moment du déploiement du client), il convient de détecter les problèmes de conception plus tôt que cela - avant que le code ne soit réellement écrit.

Avec l'examen par les pairs, le risque de perte de contrôle est minime. Tout le temps, deux personnes connaissent le même code. Ils doivent commuter de temps en temps, ils doivent donc être attentifs tout le temps pour garder une trace du code.

Il est logique d’avoir un développeur habile et un novice qui travaillent ensemble. À première vue, cela semble inefficace, mais ce n’est pas le cas. En fait, il y a moins de bugs et cela prend moins de temps pour les corriger. En outre, les débutants apprendront beaucoup plus vite.

Pour éviter une mauvaise conception, cela devrait être fait avant le codage. S'il y a des modifications / améliorations / nouveaux éléments de conception considérables, ceux-ci doivent être revus avant le début du codage. Lorsque le design est complètement développé, il ne reste plus grand chose à faire. Réviser le code sera plus facile et prendra moins de temps.

Je conviens qu’il n’est pas essentiel de réviser le code avant de s’engager que si le code est produit par un développeur expérimenté, qui a déjà prouvé ses compétences. Mais s'il y a un débutant, le code doit être examiné avant de s'engager: le relecteur doit s'asseoir à côté du développeur et expliquer chaque modification ou amélioration apportée par celui-ci.

Les révisions bénéficient des pré-et post-validations.

Pre-review commit

• Donne aux réviseurs l'assurance qu'ils examinent la dernière révision de l'auteur.
• Permet de s'assurer que tout le monde examine le même code.
• Donne une référence pour la comparaison une fois que les révisions apportées aux éléments de révision sont terminées.

Aucune validation en cours lors de l'examen

J'ai utilisé des outils Atlassian et j'ai vu des commits en cours se produire lors de l'examen. C'est déroutant pour les critiques, donc je le déconseille.

Révisions postérieures à l'examen

Une fois que les réviseurs ont complété leurs commentaires, verbalement ou par écrit, le modérateur doit s’assurer que l’auteur apporte les modifications demandées. Parfois, les réviseurs ou l’auteur peuvent ne pas être d’accord sur l’opportunité de désigner un élément de révision comme une faute, une suggestion ou une enquête. Pour résoudre les désaccords et garantir que les éléments d'enquête sont correctement résolus, l'équipe de révision dépend du jugement du modérateur.

D'après mon expérience avec environ 100 inspections de code, lorsque les réviseurs peuvent faire référence à une norme de codage non ambiguë et pour la plupart des types de logique et autres erreurs de programmation, les résultats de la révision sont généralement clairs. De temps en temps, il y a un débat sur le tatouage ou un point de style peut dégénérer en argument. Cependant, donner le pouvoir de décision au modérateur évite l'impasse.

Engagement post-révision

• Donne au modérateur et aux autres relecteurs un point de données à comparer avec le commit de pré-relecture.
• Fournit des mesures permettant de juger à la fois de la valeur et du succès de la révision lors de la suppression des défauts et de l'amélioration du code.

Cela dépend de la composition de votre équipe. Pour une équipe relativement expérimentée qui maîtrise bien les petits commits fréquents, puis une révision post-validation, il suffit de regarder le code dans les yeux. Pour les validations plus importantes et plus complexes et / ou pour les développeurs moins expérimentés, les examens préalables à la validation visant à résoudre les problèmes avant qu'ils ne surviennent semblent plus prudents.

Dans le même ordre d'idées, le fait d'avoir un bon processus de CI et / ou des enregistrements sécurisés réduit le besoin de révisions avant l'engagement (et peut-être post-engagement pour nombre d'entre eux).

Mes collègues et moi-même avons récemment effectué des recherches scientifiques sur ce sujet. J'aimerais donc ajouter certaines de nos idées, même si cette question est plutôt ancienne. Nous avons construit un modèle de simulation d'un processus / d'une équipe de développement Kanban agile et comparé les révisions pré-engagées et post-validations pour un grand nombre de situations différentes (nombre différent de membres de l'équipe, niveaux de compétences différents, ...). Après trois années de développement (simulé), nous avons examiné les résultats et avons recherché des différences en termes d'efficacité (points de récit finis), de qualité (bugs décelés par les clients) et de temps de cycle (temps écoulé entre le début et la livraison d'une user story). . Nos conclusions sont les suivantes:

• Les différences d'efficacité et de qualité sont négligeables dans de nombreux cas. Lorsqu'ils ne le sont pas, la révision post-validation présente certains avantages en termes de qualité (d'autres développeurs agissent en quelque sorte comme des "bêta-testeurs"). Pour plus d'efficacité, le post-engagement présente certains avantages pour les petites équipes et le pré-engagement présente certains avantages pour les grandes équipes ou les moins qualifiés.
• La révision préalable à la validation peut entraîner des durées de cycle plus longues, lorsque la révision retarde le début des tâches dépendantes.

Nous en avons déduit les règles heuristiques suivantes:

• Lorsque vous avez mis en place un processus d’examen du code, ne changez rien, cela ne vaut probablement pas la peine.
  • sauf si vous avez des problèmes avec le temps de cycle => Basculer vers Post
  • Ou des problèmes non résolus perturbent trop souvent vos développeurs => Basculer vers Pré
• Si vous ne faites pas encore d'avis
  • Utilisez le pré-engagement si l’un de ces avantages s’applique pour vous.
    • La révision préalable à la validation permet aux utilisateurs extérieurs sans droits de validation de contribuer à des projets open source
    • Si la révision préalable à la validation est basée sur un outil, elle impose une discipline de révision dans les équipes dont le processus est respecté.
    • La vérification avant validation empêche facilement la livraison des modifications non examinées, ce qui est parfait pour un déploiement continu / des cycles de publication très courts.
  • Utilisez le pré-engagement si votre équipe est nombreuse et que vous pouvez surmonter ou résoudre les problèmes pendant le temps du cycle.
  • Sinon (par exemple, petite équipe industrielle qualifiée), utilisez post-commit
• Cherchez des combinaisons qui vous donnent les avantages des deux mondes (nous ne les avons pas étudiées formellement)

Le document de recherche complet est disponible à l' adresse suivante : http://dx.doi.org/10.1145/2904354.2904362 ou sur mon site Web: http://tobias-baum.de

À mon avis, l'examen par les pairs du code fonctionne mieux si cela est fait après la validation.

Je recommanderais d'ajuster votre stratégie de branchement. L'utilisation d'une branche de développeur ou d'une branche de fonctionnalités présente un certain nombre d'avantages, dont le plus simple est de faciliter les révisions de code post-commit.

Un outil comme Crucible lissera et automatisera le processus de révision. Vous pouvez sélectionner un ou plusieurs ensembles de modifications validés à inclure dans la révision. Crucible, il montrera quels fichiers ont été touchés dans les changesets sélectionnés, gardera une trace des fichiers déjà lus par chaque relecteur (affichant un% de complétude globale) et permettant aux relecteurs de faire facilement des commentaires.

http://www.atlassian.com/software/crucible/overview

Quelques autres avantages des branches utilisateur / fonctionnalité:

• Les développeurs bénéficient des avantages du contrôle de version (modifications de sauvegarde, restauration à partir de l’historique, modifications des différences) sans craindre de casser le système à tout le monde.
• Les modifications qui causent des défauts ou ne sont pas terminées à temps peuvent être annulées, redéfinies par priorité ou mises en attente si nécessaire.

Pour les développeurs inexpérimentés, une consultation régulière avec un mentor et / ou une programmation en binôme est une bonne idée, mais je ne considérerais pas cela comme une "révision de code".

Tous les deux. (Genre de.)

Vous devriez passer en revue votre propre code de manière sommaire avant de le commettre. Dans Git, je pense que la zone de rassemblement est géniale. Après avoir mis en scène mes modifications, je cours git diff --cachedpour voir tout ce qui est mis en scène. J'utilise cette opportunité pour m'assurer de ne pas archiver les fichiers n'appartenant pas (artefacts de construction, journaux, etc.) et de m'assurer que je n'ai pas de code de débogage ni de code important commenté en dehors. (Si je fais quelque chose que je sais que je ne veux pas enregistrer, je laisse généralement un commentaire en majuscule pour que je le reconnaisse lors de la mise en scène.)

Cela dit, votre examen de code par les pairs doit généralement être effectué après votre validation, en supposant que vous travailliez sur une branche. C’est le moyen le plus simple de s’assurer que tout le monde examine la bonne chose, et s’il ya des problèmes majeurs, il n’est alors pas grave de les corriger sur votre branche ou de les supprimer et de tout recommencer. Si vous effectuez des révisions de code de manière asynchrone (c.-à-d. À l'aide de Google Code ou d'Atlassian Crucible), vous pouvez facilement changer de branche et travailler sur autre chose sans avoir à garder séparément la trace de tous vos différents correctifs / diffs actuellement en révision pendant quelques jours.

Si vous ne travaillez pas sur une branche thématique, vous devriez le faire . Cela réduit le stress et les tracas et rend la planification de la libération beaucoup moins stressante et compliquée.

Edit: Je devrais également ajouter que vous devriez être en train de réviser le code après les tests, ce qui est un autre argument en faveur de la validation du code en premier. Vous ne voulez pas que votre groupe de test manipule des dizaines de correctifs / diffs de tous les programmeurs, puis claque des bogues simplement parce qu'ils ont appliqué le mauvais correctif au mauvais endroit.

Programmation couplée à 100% (peu importe votre niveau d'âge) avec beaucoup de petits commits et un système de CI qui s'appuie sur CHAQUE commit (avec des tests automatisés comprenant des unités, une intégration et des fonctionnels chaque fois que possible). Examens post-engagement pour des modifications importantes ou risquées. Si vous devez avoir une sorte de critique gated / pre-commit, Gerrit fonctionne.

L'avantage de la révision du code à l'enregistrement (vérification par un copain) est un retour immédiat, avant que de gros morceaux de code ne soient terminés.

L'inconvénient de la révision du code à l'enregistrement est que cela peut décourager les personnes de s'enregistrer jusqu'à ce que de longues périodes de code soient terminées. Si cela se produit, l'avantage est totalement annulé.

Ce qui rend la tâche plus difficile, c’est que tous les développeurs ne sont pas identiques. Les solutions simples ne fonctionnent pas pour tous les programmeurs . Les solutions simples sont:

• Programmation par paire obligatoire, qui permet de faire des enregistrements fréquents car le copain est juste à côté de vous. Cela ignore que la programmation par paires ne fonctionne pas toujours pour tout le monde. Effectuée correctement, la programmation en binôme peut également être très fatigante, de sorte que ce n'est pas forcément quelque chose à faire toute la journée.

• Les branches de développeur, le code est uniquement revu et vérifié dans la branche principale une fois terminé. Certains développeurs sont enclins à travailler dans le secret et, après une semaine, ils proposent un code qui peut être soumis ou non à une révision en raison de problèmes fondamentaux qui auraient pu être repérés plus tôt.

• Revoir à chaque enregistrement, ce qui garantit des révisions fréquentes. Certains développeurs sont oublieux et s'appuient sur des enregistrements très fréquents, ce qui signifie que d'autres doivent effectuer des révisions de code toutes les 15 minutes.

• Passez en revue à une heure indéterminée après l'enregistrement. Les révisions seront repoussées plus loin au moment critique. Le code qui dépend du code déjà validé mais pas encore révisé sera validé. Les examens signaleront les problèmes et les problèmes seront placés dans l'arriéré pour être corrigés "plus tard". Ok, j'ai menti: Ce n'est pas une solution simple, ce n'est pas une solution du tout. Réviser à un moment donné après que l'enregistrement fonctionne

En pratique, vous réalisez ce travail en le rendant encore plus simple et plus complexe en même temps. Vous définissez des directives simples et laissez chaque équipe de développement définir en équipe ce qu'elle doit faire pour suivre ces instructions. Un exemple de telles directives est:

• Le travail est divisé en tâches qui devraient prendre moins d'une journée.
• Une tâche n'est pas terminée si le code (le cas échéant) n'a pas été archivé.
• Une tâche n'est pas terminée si le code (le cas échéant) n'a pas été revu.

De nombreuses formes alternatives de telles directives sont possibles. Concentrez-vous sur ce que vous voulez réellement (code révisé par les pairs, progression du travail observable, responsabilité) et laissez l'équipe déterminer comment elle peut vous donner ce qu'elle veut.

nous faisons en fait un hybride sur LedgerSMB. Les committers commettent des modifications qui sont passées en revue après. Les non-committers soumettent les modifications aux committers qui doivent être examinées avant. Cela tend à signifier deux niveaux d'examen. Vous devez d’abord vous faire réviser par un mentor. Ensuite, ce mentor fait réviser le code une deuxième fois après l'avoir approuvé et la restitution des commentaires. Les nouveaux committers passent généralement beaucoup de temps à examiner les commits des autres.

Ça marche plutôt bien. Cependant, le point essentiel est qu’un examen après est généralement plus superficiel qu’un examen précédent. Vous voulez donc vous assurer que cet examen est réservé à ceux qui ont fait leurs preuves. Mais si vous avez un examen à deux niveaux pour les nouveaux venus, cela signifie que les problèmes sont plus susceptibles d'être résolus et que des discussions ont eu lieu.

Commit où? Il y a une branche que j'ai créée pour faire du travail. Je m'engage à cette branche chaque fois que j'en ai envie. Ce n'est l'affaire de personne. Puis, à un moment donné, cette branche est intégrée à une branche de développement. Et quelque part entre les deux est une révision du code.

Le critique commente évidemment après que je me suis engagé dans ma branche. Il n'est pas assis à mon bureau, il ne peut donc pas l'examiner avant que je ne m'engage dans ma branche. Et il passe en revue avant la fusion et s'engage dans la branche de développement.

Juste ne faites pas du tout des revues de code. Soit vous pensez que vos développeurs sont capables d’écrire un bon code, soit vous devez vous en débarrasser. Les erreurs de logique doivent être capturées par vos tests automatisés. Les erreurs de style doivent être capturées par les outils d’analyse de la charpie et de la statique.

Faire participer des êtres humains à ce qui devrait être des processus automatiques n'est que du gaspillage.