MTI·Training

Jour 25 Day 25 · vendredi 4 septembre 2026 Friday 4 September 2026 Qualité Fondamental

La code review : donner et recevoir Code review: giving and receiving

Reviewer une PR et encaisser des commentaires sans le prendre personnellement : la compétence d'équipe que les recruteurs sondent systématiquement en entretien de stage. Reviewing a PR and taking comments without taking them personally: the teamwork skill recruiters systematically probe in internship interviews.

L’essentiel

La code review est la relecture d’un changement de code par au moins un autre développeur avant son merge. Premier réflexe à corriger avant l’entretien : la review ne sert pas d’abord à « trouver des bugs ». Elle en attrape, mais sa vraie valeur est ailleurs :

  • Partage de connaissance — le reviewer découvre une partie du code qu’il n’a pas écrite, l’auteur reçoit du contexte qu’il n’avait pas. C’est l’assurance contre le bus factor : personne ne doit être le seul à comprendre un module.
  • Cohérence du codebase — mêmes patterns, mêmes conventions d’architecture, mêmes façons de gérer les erreurs. Dix développeurs, un seul style de projet.
  • Qualité de conception — une deuxième paire d’yeux voit l’API maladroite, le cas limite oublié, le test manquant, le problème de sécurité.

Et ce à quoi elle ne sert pas : le style. Indentation, guillemets, ordre des imports — les linters et formatters (ESLint, Prettier, Black, clang-format) font ça automatiquement en CI. Un humain qui commente une virgule gaspille le temps de deux personnes ; en entretien, dire « le style, c’est le travail du linter, pas du reviewer » marque immédiatement des points.

La qualité d’une review se joue dans la qualité de ses commentaires :

Mauvais commentaireBon commentaire
« C’est faux. »« blocking: parseInt(s) sans radix parse "08" comme octal sur les vieux runtimes — ajouter , 10. »
« Pourquoi tu as fait ça ?? »« question: quel cas d’usage couvre ce fallback ? Je ne le vois pas testé. »
« Renomme ta variable. »« nit: data → invoices dirait ce que contient le tableau. Non bloquant. »
« Moi j’aurais fait une Map. »« suggestion: une Map éviterait le lookup O(n) dans la boucle — à mesurer si la liste grossit. »

La différence tient en un mot : actionnable. Le bon commentaire dit ce qui pose problème, pourquoi, et ce qui débloquerait la situation.

Comment ça marche

Le cycle de vie d’une PR tient dans un schéma — noter que la boucle de re-review est la partie coûteuse, celle que les petites PR raccourcissent :

code ─▶ auto-review ─▶ PR ─▶ review ─▶ approve ─▶ merge
                             ▲  │
                   re-review │  │ changes requested
                             │  ▼
                      push des correctifs

Côté reviewer, la méthode en trois temps :

  1. Comprendre l’intention d’abord. Lire le titre, la description, le ticket lié — avant la première ligne de diff. Reviewer du code sans savoir ce qu’il essaie d’accomplir, c’est corriger une dictée sans connaître le sujet.
  2. Du général au particulier. L’approche est-elle la bonne ? Le changement est-il au bon endroit ? Seulement ensuite : la logique ligne à ligne, les cas limites, les tests. Un « cette approche ne tiendra pas la charge » vaut plus que vingt remarques de détail sur un code qui sera réécrit.
  3. Commenter en distinguant deux niveaux. Ce qui bloque le merge (bug, faille, données perdues, incohérence d’architecture) et ce qui n’est qu’une suggestion ou une préférence. Tout mettre au même niveau noie les vrais problèmes et épuise l’auteur.

Et une règle de ton : des questions plutôt que des ordres. « Que se passe-t-il si items est vide ? » ouvre une discussion ; « gère le tableau vide » présuppose que le reviewer a raison — or il n’a pas tout le contexte. La question laisse la porte ouverte à « c’est garanti non vide par la validation en amont », réponse qui clôt le sujet en dix secondes.

Les conventional comments (conventionalcomments.org) formalisent ce double niveau avec un préfixe par commentaire : blocking: (à résoudre avant merge), question: (besoin d’une réponse, pas forcément d’un changement), suggestion: (amélioration proposée), nit: (détail mineur, jamais bloquant), praise: (souligner ce qui est bien fait — oui, ça se fait, et ça change l’ambiance d’une review). Le préfixe supprime toute ambiguïté : l’auteur sait instantanément ce qui l’empêche de merger.

Un extrait de review annoté avec ces commentaires types :

# PR « Validation de l'email à l'inscription » — extrait
# annoté avec les commentaires du reviewer

- if (email.includes("@")) {
+ if (EMAIL_REGEX.test(email)) {
    await createUser(email);
  }
# blocking: EMAIL_REGEX n'est importé nulle part, la CI
# est rouge — la PR ne peut pas partir en l'état.

+ console.log("created: " + email);
# blocking: on logge une donnée personnelle en clair.
# Peut-on logger l'id utilisateur plutôt que l'email ?

+ const t = Date.now();
# question: à quoi sert ce timestamp ? Je ne le vois
# utilisé nulle part — un reste de debug ?

- function create_user(email) {
+ async function createUser(email) {
# nit: renommage bienvenu mais hors sujet de la PR —
# une PR séparée la prochaine fois ? Non bloquant.

Concepts clés à maîtriser

  • Petites PR. La qualité d’une review s’effondre avec la taille du diff : au-delà de quelques centaines de lignes, le reviewer survole et approuve — c’est l’effet « LGTM » (looks good to me). Une grosse feature se découpe en PR successives : d’abord le modèle de données, puis la logique, puis l’UI.
  • Auto-review avant de soumettre. Relire son propre diff dans l’interface de PR, comme si on était le reviewer : on y attrape le console.log oublié, le fichier commité par erreur, le commentaire mort. Compléter avec une description qui donne le contexte (quoi, pourquoi, comment tester). Chaque minute d’auto-review économise un aller-retour de review — soit des heures de latence.
  • Bloquant vs non-bloquant. Un reviewer qui bloque une PR pour un nommage impose sa préférence ; un reviewer qui laisse passer une injection SQL pour ne pas froisser ne fait pas son travail. Savoir classer chaque remarque dans la bonne catégorie est la compétence de review.
  • Recevoir une review. Trois règles : ce n’est pas personnel (on review le code, pas la personne — et le code, dans trois mois, ne sera plus « le vôtre » mais celui de l’équipe) ; répondre à tout (chaque commentaire reçoit un fix, une réponse ou un ticket — jamais un silence) ; le désaccord s’argumente (« je garde cette approche parce que X » est légitime, appliquer en silence un changement qu’on juge mauvais ne l’est pas).
  • Le standard de merge. La règle du guide Google : on approuve dès que le changement améliore la santé globale du code, même s’il n’est pas parfait. Exiger la perfection paralyse l’équipe ; le « moi j’aurais fait autrement » n’est pas un motif de blocage si l’approche de l’auteur fonctionne et reste cohérente.

💡 La PR de 200 lignes max — c’est l’ordre de grandeur à retenir (les études classiques de SmartBear situent le décrochage d’attention vers 400 lignes ; viser 200 garde de la marge). Une PR de 200 lignes est relue en profondeur en vingt minutes ; une PR de 2000 lignes reçoit un « LGTM » en trois. Petite PR = review meilleure, feedback plus rapide, conflits de merge réduits, revert facile.

🎤 En entretien — « comment réagis-tu à une review négative ? » est une question piège classique : le recruteur teste l’ego, pas la technique. La réponse attendue : je dissocie le code de ma personne, je lis tous les commentaires avant de répondre, je corrige ce qui est fondé, et quand je ne suis pas d’accord je le dis avec des arguments — un désaccord technique argumenté est une marque de maturité, pas d’arrogance. Bonus : mentionner qu’une review dense signifie que le reviewer a pris le temps de lire, et que c’est préférable à un LGTM distrait.

En entretien

« À quoi sert une code review ? » — Trois choses : partager la connaissance (personne n’est le seul à connaître un module), garantir la cohérence du codebase, et améliorer la conception via une deuxième paire d’yeux. Préciser ce qu’elle ne fait pas : le style, automatisé par les linters en CI. Réduire la review à « chercher des bugs » est la réponse faible.

« Comment écris-tu un bon commentaire de review ? » — Actionnable : ce qui pose problème, pourquoi, et une piste de sortie. Sous forme de question quand je n’ai pas tout le contexte. Étiqueté par gravité — blocking: vs nit: — pour que l’auteur sache ce qui empêche le merge. Et jamais sur le style : le linter s’en charge.

« Un reviewer te demande un changement que tu trouves injustifié, tu fais quoi ? » — Je réponds au commentaire avec mes arguments (contrainte, mesure, contexte qu’il n’a pas). S’il maintient avec de bonnes raisons, j’applique ; si le désaccord persiste sur un point non bloquant, la convention d’équipe ou l’avis d’un tiers tranche. Ce que je ne fais jamais : ignorer le commentaire, ou appliquer en silence en pensant que c’est faux.

« Quelle taille idéale pour une PR et pourquoi ? » — L’ordre de grandeur : 200 lignes, quelques centaines maximum. Au-delà, l’attention du reviewer décroche et la review devient un survol. Une grosse feature se découpe en PR empilées, chacune relisible en une session.

« Que fais-tu avant de soumettre une PR ? » — Une auto-review du diff complet dans l’interface, comme si j’étais le reviewer : debug oublié, fichiers parasites, code mort. Puis une description avec le contexte, le lien vers le ticket et comment tester. Une PR qui arrive propre économise un aller-retour complet de review.

Pièges & idées reçues

⚠️ L’effet LGTM — le piège le plus courant en équipe : les grosses PR et les reviews « pour la forme » s’auto-renforcent. Plus la PR est grosse, moins elle est vraiment lue, plus les approbations deviennent des tampons — et plus la review perd sa crédibilité, donc son utilité. La discipline des petites PR n’est pas du confort : c’est ce qui maintient la review vivante.

  • « La review sert à vérifier le style » — non : linters et formatters le font en CI, sans fatigue ni débat. Si votre équipe débat des guillemets en review, il manque un outil, pas de la rigueur.
  • « Un commentaire de review est un ordre » — non : c’est le début d’une conversation. L’auteur peut répondre, argumenter, refuser avec de bonnes raisons. Seuls les blocking: conditionnent le merge.
  • « Une review sévère = le reviewer me juge » — la review porte sur le code, jamais sur la personne. Symétriquement, côté reviewer : bannir le « tu » accusateur (« tu as oublié… ») au profit du code (« ce chemin ne gère pas le cas vide »).
  • Laisser des commentaires sans réponse — merger en ignorant des remarques détruit la confiance. Chaque commentaire mérite un fix, une réponse, ou un ticket de suivi explicite.
  • Exiger la perfection — le standard, c’est « mieux qu’avant », pas « parfait ». Bloquer une PR fonctionnelle pour des préférences personnelles est un abus de review.

Pour aller plus loin

The essentials

A code review is the reading of a code change by at least one other developer before it gets merged. First reflex to fix before the interview: review is not primarily about “finding bugs”. It does catch some, but its real value lies elsewhere:

  • Knowledge sharing — the reviewer discovers a part of the code they didn’t write, the author receives context they didn’t have. It’s the insurance against the bus factor: nobody should be the only person who understands a module.
  • Codebase consistency — same patterns, same architectural conventions, same ways of handling errors. Ten developers, one project style.
  • Design quality — a second pair of eyes spots the awkward API, the forgotten edge case, the missing test, the security issue.

And what it is not for: style. Indentation, quotes, import order — linters and formatters (ESLint, Prettier, Black, clang-format) handle that automatically in CI. A human commenting on a comma wastes two people’s time; in an interview, saying “style is the linter’s job, not the reviewer’s” scores points immediately.

The quality of a review comes down to the quality of its comments:

Bad commentGood comment
“This is wrong.”“blocking: parseInt(s) without a radix parses "08" as octal on old runtimes — add , 10.”
“Why did you do that??”“question: which use case does this fallback cover? I don’t see it tested.”
“Rename your variable.”“nit: data → invoices would say what the array contains. Non-blocking.”
“I would have used a Map.”“suggestion: a Map would avoid the O(n) lookup in the loop — worth measuring if the list grows.”

The difference fits in one word: actionable. A good comment says what the problem is, why, and what would unblock the situation.

How it works

The lifecycle of a PR fits in one diagram — note that the re-review loop is the expensive part, the one small PRs shorten:

code ─▶ self-review ─▶ PR ─▶ review ─▶ approve ─▶ merge
                             ▲  │
                   re-review │  │ changes requested
                             │  ▼
                        push the fixes

On the reviewer’s side, the method in three steps:

  1. Understand the intent first. Read the title, the description, the linked ticket — before the first line of diff. Reviewing code without knowing what it’s trying to accomplish is like grading an essay without knowing the topic.
  2. From general to specific. Is the approach the right one? Is the change in the right place? Only then: line-by-line logic, edge cases, tests. One “this approach won’t hold under load” is worth more than twenty detail remarks on code that will be rewritten.
  3. Comment on two distinct levels. What blocks the merge (bug, vulnerability, data loss, architectural inconsistency) and what is merely a suggestion or a preference. Putting everything on the same level drowns the real problems and exhausts the author.

And one rule of tone: questions rather than orders. “What happens if items is empty?” opens a discussion; “handle the empty array” assumes the reviewer is right — yet they don’t have all the context. The question leaves the door open to “it’s guaranteed non-empty by the upstream validation”, an answer that closes the topic in ten seconds.

Conventional comments (conventionalcomments.org) formalize this two-level distinction with a prefix per comment: blocking: (must be resolved before merge), question: (needs an answer, not necessarily a change), suggestion: (proposed improvement), nit: (minor detail, never blocking), praise: (highlighting what’s well done — yes, that’s a thing, and it changes the mood of a review). The prefix removes all ambiguity: the author instantly knows what prevents them from merging.

An annotated review excerpt with these typical comments:

# PR "Email validation at signup" — excerpt
# annotated with the reviewer's comments

- if (email.includes("@")) {
+ if (EMAIL_REGEX.test(email)) {
    await createUser(email);
  }
# blocking: EMAIL_REGEX is not imported anywhere, CI
# is red — this PR cannot ship as is.

+ console.log("created: " + email);
# blocking: we're logging personal data in plain text.
# Could we log the user id instead of the email?

+ const t = Date.now();
# question: what is this timestamp for? I don't see
# it used anywhere — a debug leftover?

- function create_user(email) {
+ async function createUser(email) {
# nit: welcome rename but out of scope for this PR —
# a separate PR next time? Non-blocking.

Key concepts to master

  • Small PRs. Review quality collapses with diff size: beyond a few hundred lines, the reviewer skims and approves — the “LGTM” effect (looks good to me). A big feature gets split into successive PRs: first the data model, then the logic, then the UI.
  • Self-review before submitting. Re-read your own diff in the PR interface, as if you were the reviewer: you’ll catch the forgotten console.log, the file committed by mistake, the dead comment. Complete it with a description that gives context (what, why, how to test). Every minute of self-review saves a review round-trip — that is, hours of latency.
  • Blocking vs non-blocking. A reviewer who blocks a PR over a name is imposing a preference; a reviewer who lets a SQL injection through to avoid hurting feelings is not doing their job. Sorting each remark into the right category is the core review skill.
  • Receiving a review. Three rules: it’s not personal (the code is reviewed, not the person — and in three months the code will no longer be “yours” but the team’s); respond to everything (each comment gets a fix, an answer, or a ticket — never silence); disagreement comes with arguments (“I’m keeping this approach because X” is legitimate, silently applying a change you believe is wrong is not).
  • The merge standard. The rule from Google’s guide: approve as soon as the change improves the overall health of the code, even if it isn’t perfect. Demanding perfection paralyzes the team; “I would have done it differently” is not grounds for blocking if the author’s approach works and stays consistent.

💡 The 200-line PR — that’s the order of magnitude to remember (the classic SmartBear studies place the attention drop-off around 400 lines; aiming for 200 keeps a margin). A 200-line PR gets a deep read in twenty minutes; a 2000-line PR gets an “LGTM” in three. Small PR = better review, faster feedback, fewer merge conflicts, easy revert.

🎤 In an interview — “how do you react to a negative review?” is a classic trap question: the recruiter is testing ego, not technique. The expected answer: I separate the code from my person, I read all the comments before replying, I fix what’s justified, and when I disagree I say so with arguments — an argued technical disagreement is a sign of maturity, not arrogance. Bonus: mention that a dense review means the reviewer took the time to actually read, which beats a distracted LGTM.

In an interview

“What is a code review for?” — Three things: sharing knowledge (nobody is the only one who knows a module), keeping the codebase consistent, and improving design through a second pair of eyes. Specify what it does not do: style, automated by linters in CI. Reducing review to “finding bugs” is the weak answer.

“How do you write a good review comment?” — Actionable: what the problem is, why, and a way out. Phrased as a question when I don’t have all the context. Labeled by severity — blocking: vs nit: — so the author knows what prevents the merge. And never about style: the linter handles that.

“A reviewer requests a change you find unjustified — what do you do?” — I reply to the comment with my arguments (a constraint, a measurement, context they don’t have). If they hold their ground with good reasons, I apply it; if the disagreement persists on a non-blocking point, the team convention or a third opinion settles it. What I never do: ignore the comment, or apply it silently while believing it’s wrong.

“What’s the ideal size for a PR and why?” — The order of magnitude: 200 lines, a few hundred at most. Beyond that, the reviewer’s attention drops and the review becomes a skim. A big feature gets split into stacked PRs, each readable in one session.

“What do you do before submitting a PR?” — A self-review of the full diff in the interface, as if I were the reviewer: forgotten debug code, stray files, dead code. Then a description with the context, the link to the ticket, and how to test. A PR that arrives clean saves a full review round-trip.

Pitfalls & misconceptions

⚠️ The LGTM effect — the most common team-level trap: big PRs and rubber-stamp reviews reinforce each other. The bigger the PR, the less it’s actually read, the more approvals become stamps — and the more the review loses credibility, and therefore usefulness. The discipline of small PRs is not comfort: it’s what keeps reviews alive.

  • “Review is there to check style” — no: linters and formatters do it in CI, without fatigue or debate. If your team argues about quotes in review, a tool is missing, not rigor.
  • “A review comment is an order” — no: it’s the start of a conversation. The author can reply, argue, refuse with good reasons. Only blocking: comments condition the merge.
  • “A harsh review = the reviewer judging me” — the review targets the code, never the person. Symmetrically, on the reviewer’s side: ban the accusatory “you” (“you forgot…”) in favor of the code (“this path doesn’t handle the empty case”).
  • Leaving comments unanswered — merging while ignoring remarks destroys trust. Every comment deserves a fix, an answer, or an explicit follow-up ticket.
  • Demanding perfection — the standard is “better than before”, not “perfect”. Blocking a working PR over personal preferences is review abuse.

Going further

S'entraîner sur ce sujet → Practice this topic →