knowledge/code-review.md

# code-review.md — Revues, pull requests, collaboration

Ce fichier couvre la partie humaine du code : comment je lis le code des autres, comment je soumets le mien, comment un diff devient du merge propre.

La posture s'est aussi construite côté **enseignement** : je donne des cours à teachings.samuel-bouchet.fr (back-end, front-end, jeu vidéo, algorithmique, JavaScript) et j'y évalue du code régulièrement. Reviewer en mentor et reviewer en pair sont deux postures différentes, mais la même rigueur s'applique — et le même respect.

## Posture générale

Une revue de code n'est pas un tribunal. C'est :

- **un transfert de contexte** : le relecteur apprend ce qui a changé, et pourquoi ;
- **une seconde paire d'yeux** : certains bugs ne se voient que depuis un autre cerveau ;
- **un alignement sur les conventions** : on grandit ensemble comme équipe ;
- **une occasion de mentorat dans les deux sens** — le senior apprend aussi des juniors.

Je pars du principe que l'auteur a fait de son mieux, que mes remarques sont des propositions, et que le dernier mot revient à l'auteur et à la conversation — pas au relecteur par défaut.

## Ma checklist mentale en revue

Quand j'ouvre un diff, je lis dans cet ordre :

### 1. Est-ce que je comprends l'intention ?

Avant de lire le code, je lis **la description de la PR**. Si elle n'explique pas :

- pourquoi ce changement existe,
- ce qu'il fait à haut niveau,
- ce qu'il ne fait pas (scope),

… je demande une reformulation avant d'aller plus loin. Lire du code sans connaître l'intention, c'est chercher des bugs invisibles.

### 2. Le scope est-il raisonnable ?

- Une PR qui fait trois choses non liées est plus dure à reviewer que trois PR d'une chose chacune. Je demande un split quand c'est pertinent.
- Un refactor massif qui traîne sous un fix de bug ? Le refactor doit être extrait dans sa propre PR.
- Un changement de convention global qui modifie 40 fichiers ? Mérite sa propre PR, documentée, pour que le review ne se noie pas dedans.

### 3. Est-ce que l'architecture tient ?

- **Séparation des couches** : la logique métier ne fuit-elle pas dans la présentation ? Unity n'est-il pas importé dans `Engine/` ? (Voir [`coding.md`](coding.md) et [`unity.md`](unity.md).)
- **Sources de vérité** : un nouveau champ est-il déjà calculable depuis un autre ? Un nouveau Signal est-il vraiment nécessaire ?
- **Event sourcing** : les mutations passent-elles toutes par des GameEvent ? (Voir [`reactive-state.md`](reactive-state.md).)
- **Nullable** : les `!` sont-ils justifiés, ou est-ce qu'on étouffe un warning ?

### 4. Est-ce que c'est lisible ?

- Le nom des classes, méthodes, champs, dit-il ce qu'il fait ?
- Est-ce qu'un lecteur qui arrive sans contexte comprend le fichier en une lecture ?
- Y a-t-il des commentaires nécessaires (pourquoi complexe, pourquoi ce choix), et pas de commentaires inutiles (paraphrase du code) ?

### 5. Est-ce que c'est testé ?

- Tests unitaires sur la logique métier (Engine) ? Si non, pourquoi pas ?
- Un test qui reproduirait le bug corrigé ?
- Un test qui documente le nouveau comportement ?
- Les tests suivent-ils la convention `MethodOrEvent_Scenario_ExpectedResult` et la structure Arrange/Act/Assert ?

### 6. Y a-t-il des pièges techniques ?

- Allocations inutiles en hot-path ?
- Tâches async sans cancellation token ?
- `Find*` dans un `Update()` ?
- Mutation d'une collection pendant son itération ?
- Sérialisation cassée : un champ `[Key(N)]` renuméroté sans versioning ?
- Assertions en éditeur qui masquent un bug en prod ?

### 7. Doc à jour ?

- Le `CLAUDE.md` / `README.md` reflète-t-il toujours la réalité ?
- Si la PR change un pattern documenté, la doc doit suivre.
- Un nouveau GameEvent non documenté dans une state diagram mérite une mise à jour du mermaid.

## Écrire un commentaire de review utile

Les règles que je respecte (et que j'espère qu'on me rend) :

- **Être précis** : pointer un numéro de ligne, citer le code, proposer une alternative concrète quand c'est possible.
- **Séparer "blocker" de "suggestion"** : préfixer `blocker:`, `suggestion:`, `nit:`, `question:`, `praise:` pour que l'auteur sache ce qui est un vrai retour et ce qui est juste une pensée.
  - **blocker** : ne doit pas merger sans ça.
  - **suggestion** : pense à le faire si possible, mais merge OK sinon.
  - **nit** (nitpick) : esthétique, optionnel.
  - **question** : je veux comprendre, pas un retour.
  - **praise** : ça me plaît et je veux le dire.
- **Commenter le code, pas la personne**. "Ce nommage est ambigu" plutôt que "tu as mal nommé".
- **Proposer, pas imposer**. Un "et si on faisait X ?" lance une conversation ; un "fais X" ferme le dialogue.
- **Accepter qu'on puisse avoir tort**. Si l'auteur explique pourquoi son choix tient, je peux revenir sur mon retour sans drame.

## Soumettre une PR qu'on a envie de review

### Le titre

- Court (< 70 caractères), au présent, descriptif.
- Convention informelle : `feat:`, `fix:`, `refactor:`, `docs:`, `test:`, `chore:`, `perf:`.
- Pas de numéro de ticket en début (en fin si pertinent).

Exemples :
- `feat: add rune selling from bench`
- `fix: gold underflow when buying then refunding`
- `refactor: extract combat resolution into RuneCombat`
- `docs: update CLAUDE.md for new GameEvent types`

### La description

Structure type :

```markdown
## Summary
- 1 à 3 bullets sur ce qui change, le pourquoi.

## Why
Contexte. Qu'est-ce qui a motivé ? Bug rapporté, feature demandée, refactor préparatoire ?

## What
Ce qui bouge concrètement. Les fichiers/classes clés touchés, les décisions d'implémentation.

## Test plan
- [ ] Tests unitaires passent
- [ ] Scénario X testé en play mode
- [ ] Pas de régression sur Y

## Screenshots / videos
(si UI)

## Notes pour la review
Points d'attention particuliers, choix discutables, alternatives envisagées.
```

Une PR sans description est une PR qui prend deux fois plus de temps à review.

### La taille

Petite autant que possible. Mes repères :

- **< 100 lignes changées** : review en 5 minutes, merge rapide.
- **100-400 lignes** : review sérieuse, à prendre avec un café, mais faisable.
- **> 400 lignes** : si ce n'est pas un refactor mécanique, je soupçonne un problème de scope. Je demande un split.

Quand une feature mérite une grosse PR, je la découpe en PR empilées : une première qui pose les fondations, une seconde qui ajoute la logique, une troisième qui branche l'UI. Chacune mergeable indépendamment.

### Le self-review

Avant de demander une review, je relis **mon propre diff** dans l'UI de la plateforme (pas dans l'IDE). Je trouve à chaque fois :

- des `Console.WriteLine` oubliés,
- des imports inutiles,
- des commentaires de debug,
- du code mort commenté "au cas où",
- des typos dans les messages d'erreur.

Mieux vaut que je les trouve que le relecteur.

## Commits : granularité et messages

### Granularité

- **Un commit = une idée cohérente**. Pas "fin de journée" ou "wip", sauf en local temporairement.
- **Atomiques** : chaque commit doit passer les tests et compiler. Sinon `git bisect` devient impossible.
- Je rebase/squash avant de pusher quand mes commits locaux sont du brouillon ("fix", "typo", "retry"). Le dépôt reçoit des commits propres.

### Messages

Format que j'utilise :

```
<verbe au présent> <ce qui change>

<paragraphe optionnel expliquant le pourquoi et le contexte>

<refs optionnelles : fixes #123, relates to #456>
```

Exemples :

```
Add sell-from-bench action to deckbuilding

Players can now sell runes from the bench during the build phase,
mirroring the existing shop sell action. The refund is 50% of cost
rounded down, same formula as shop.
```

```
Fix gold underflow when cancelling a shop purchase

RefundShopPurchase was applying the refund before validating the
stock state, leading to negative gold when the purchase had already
been partially consumed. Now the refund only applies if the rune
is still on the bench.

Fixes #127
```

Règles :

- **Verbe au présent actif** : "Add", "Fix", "Refactor", "Remove" — comme si c'était une instruction à git.
- **Majuscule au début, pas de point final au titre** (convention usuelle).
- **Titre ≤ 72 caractères** pour l'affichage dans `git log --oneline`.
- **Ligne vide** entre titre et corps.
- **Corps en prose** qui explique le pourquoi, pas le quoi. Le quoi est dans le diff.
- **Pas de smiley**, pas de markdown dans les messages de commit (sauf listes simples si utile).

### Ce qu'un message de commit ne doit pas être

- `update file`
- `fix stuff`
- `wip`
- `.`

Quand je vois ça dans l'historique, je sais que quelqu'un s'est dépêché et qu'il faudra ouvrir le diff pour comprendre.

## Reviewer comme auteur : répondre aux commentaires

- **Répondre à tous les commentaires**, même si c'est juste "done" ou "good point, changed".
- **Ne pas prendre les retours personnellement**. Un "pourquoi ce choix ?" n'est pas un "tu as tort".
- **Pousser les changements en commits séparés** pendant la phase de review, pour que le relecteur voie ce qui a bougé. Le squash viendra au merge.
- **Rouvrir la discussion** si on n'est pas d'accord : argumenter, proposer, écouter. Pas capituler par politesse.
- **Demander une relecture explicitement** quand j'ai poussé des fixes, pour que la PR ne reste pas dans les limbes.

## Au merge

- **Squash par défaut** pour les PRs feature, pour que `main` ait un commit par feature avec un message propre.
- **Merge commit** pour les branches longues où l'historique fin mérite d'être conservé.
- **Rebase** pour les PR d'un seul commit propre — un merge fast-forward.

Je choisis au cas par cas, pas une politique rigide — mais dans la majorité des cas, squash sur une petite PR donne l'historique le plus lisible.

## Git en local : hygiène

- **Branches courtes et bien nommées** : `feat/rune-sell`, `fix/gold-underflow`, `refactor/combat-resolver`.
- **`main` jamais modifié directement** sur les projets partagés. Toujours PR.
- **Rebase sur `main`** avant de push pour éviter les merges inutiles.
- **Éviter `--force-push`** sauf sur sa propre branche et après communication. Jamais sur `main`.
- **Pas de commits générés par l'IDE ou le formatter** seuls dans l'historique — intégrés au changement fonctionnel qui les a touchés.

## Conflits de merge

Quand ils arrivent :

- Les résoudre **localement** avec le diff sous les yeux, pas dans l'UI web qui cache du contexte.
- Relancer les tests après résolution. Un conflit résolu qui ne compile plus est courant.
- Documenter en commentaire de PR si la résolution a changé le comportement initial d'une des deux branches.

## Demander de l'aide / débloquer

Quand une PR traîne :

- **Ping explicite au relecteur** après 2-3 jours de silence, poliment.
- **Proposer une session pair review** si les retours sont flous. Souvent plus rapide qu'un ping-pong asynchrone.
- **Si une PR bloque une autre**, je le dis au relecteur — c'est un signal pour prioriser.
- **Accepter de fermer une PR** qui n'aboutit pas, plutôt que la laisser pourrir. Repartir sur des bases claires est parfois plus rapide.

## CI et checks automatiques

- **Tests qui tournent à chaque PR** : unitaires d'Engine, build Unity, lint.
- **Pas de merge si la CI est rouge**. Tolérer un red build banalise le signal.
- **Checks obligatoires** pour les conventions (format, nommage) quand possible — ça évite les reviews "la virgule est mal placée".
- **Hooks pre-commit** pour le formatage et les lint rapides, pour ne pas polluer la CI avec des erreurs triviales.

## Review en solo (pair programming avec soi-même)

Quand je bosse seul sur un projet, je me force à :

- **Ouvrir une PR même pour moi-même** sur les changements significatifs — ça oblige à écrire la description, à relire, à laisser décanter.
- **Attendre 24h** avant le merge d'une grosse PR solo. Un diff relu le lendemain révèle 80% de ses défauts.
- **Me parler comme un relecteur externe** dans la description — qu'est-ce que mon moi-futur voudra savoir dans 6 mois ?

## Anti-patterns que je refuse en revue

- **Le "nit" qui bloque** — un nit est optionnel par définition.
- **La revue "LGTM" en 30 secondes sur 400 lignes** — ce n'est pas une revue, c'est un tampon.
- **Le gatekeeping** : refuser un merge parce que le code ne correspond pas exactement à la préférence personnelle du relecteur, sans argument technique.
- **La revue rétroactive** : commenter un fichier qui n'a pas changé dans la PR. À faire dans une autre PR ou un ticket.
- **Les discussions interminables sur un détail** : après 3 échanges sans convergence, soit on tranche en visio, soit on tranche au plus gradé présent, et on avance.
- **Le "ah et aussi…"** en bout de review qui réveille un scope oublié. Je préfère écrire tous mes retours en une passe et les poster ensemble.
- **Le commit "code review feedback" fourre-tout** — je préfère plusieurs petits commits thématiques que le relecteur peut suivre.