Excellence en revue de code

Transformez les revues de code, passant du gatekeeping au partage de connaissances par le biais de retours constructifs, d'une analyse systématique et d'une amélioration collaborative.

Quand utiliser cette compétence

• Réviser des pull requests et des changements de code
• Établir des standards de revue de code pour les équipes
• Mentorer les développeurs juniors par le biais des revues
• Mener des revues d'architecture
• Créer des listes de vérification et des directives de revue
• Améliorer la collaboration d'équipe
• Réduire le temps de cycle des revues de code
• Maintenir les standards de qualité du code

Principes fondamentaux

1. L'état d'esprit de la revue

Objectifs de la revue de code :

• Détecter les bugs et les cas limites
• Assurer la maintenabilité du code
• Partager les connaissances au sein de l'équipe
• Appliquer les standards de codage
• Améliorer la conception et l'architecture
• Construire la culture d'équipe

Pas les objectifs :

• Faire étalage de connaissances
• Chicaner sur le formatage (utiliser les linters)
• Bloquer la progression inutilement
• Réécrire selon vos préférences

2. Retours efficaces

Un bon retour est :

• Spécifique et actionnable
• Éducatif, non jugeant
• Focalisé sur le code, pas sur la personne
• Équilibré (féliciter aussi le bon travail)
• Priorisé (critique vs agréable à avoir)

❌ Mauvais : "C'est faux."
✅ Bon : "Cela pourrait causer une race condition quand
plusieurs utilisateurs accèdent simultanément. Envisagez
d'utiliser un mutex ici."

❌ Mauvais : "Pourquoi n'as-tu pas utilisé le pattern X ?"
✅ Bon : "As-tu envisagé le pattern Repository ? Cela
rendrait cela plus facile à tester. Voici un exemple : [lien]"

❌ Mauvais : "Renomme cette variable."
✅ Bon : "[nit] Envisage `userCount` au lieu de `uc` pour
plus de clarté. Ne bloque pas si tu préfères la garder."

3. Périmètre de la revue

Ce qu'il faut réviser :

• La correction logique et les cas limites
• Les vulnérabilités de sécurité
• Les implications de performance
• La couverture et la qualité des tests
• La gestion des erreurs
• La documentation et les commentaires
• La conception et la dénomination des API
• L'adéquation architecturale

Ce qu'il ne faut pas réviser manuellement :

• Le formatage du code (utiliser Prettier, Black, etc.)
• L'organisation des imports
• Les violations de linting
• Les fautes de frappe simples

Processus de revue

Phase 1 : Collecte de contexte (2-3 minutes)

Avant de plonger dans le code, comprenez :

1. Lisez la description du PR et l'issue liée
2. Vérifiez la taille du PR (> 400 lignes ? Demandez de diviser)
3. Vérifiez le statut CI/CD (les tests passent ?)
4. Comprenez l'exigence métier
5. Notez les décisions architecturales pertinentes

Phase 2 : Revue de haut niveau (5-10 minutes)

1. **Architecture et conception**
   - La solution répond-elle au problème ?
   - Y a-t-il des approches plus simples ?
   - Est-ce cohérent avec les patterns existants ?
   - Évoluera-t-il bien ?

2. **Organisation des fichiers**
   - Les nouveaux fichiers sont-ils au bon endroit ?
   - Le code est-il groupé logiquement ?
   - Y a-t-il des fichiers en doublon ?

3. **Stratégie de test**
   - Y a-t-il des tests ?
   - Les tests couvrent-ils les cas limites ?
   - Les tests sont-ils lisibles ?

Phase 3 : Revue ligne par ligne (10-20 minutes)

Pour chaque fichier :

1. **Logique et correction**
   - Les cas limites sont-ils gérés ?
   - Erreurs hors limites ?
   - Vérifications null/undefined ?
   - Race conditions ?

2. **Sécurité**
   - Validation des entrées ?
   - Risques d'injection SQL ?
   - Vulnérabilités XSS ?
   - Exposition de données sensibles ?

3. **Performance**
   - Requêtes N+1 ?
   - Boucles inutiles ?
   - Fuites mémoire ?
   - Opérations bloquantes ?

4. **Maintenabilité**
   - Noms de variables clairs ?
   - Les fonctions font-elles une seule chose ?
   - Code complexe commenté ?
   - Nombres magiques extraits ?

Phase 4 : Résumé et décision (2-3 minutes)

1. Résumez les préoccupations clés
2. Mettez en avant ce que vous avez aimé
3. Prenez une décision claire :
   - ✅ Approuver
   - 💬 Commenter (suggestions mineures)
   - 🔄 Demander des changements (doit traiter)
4. Offrez-vous pour faire du pair si c'est complexe

Techniques de revue

Technique 1 : La méthode de la liste de vérification

## Liste de vérification sécurité

- [ ] Entrée utilisateur validée et assainie
- [ ] Les requêtes SQL utilisent la paramétrisation
- [ ] Authentification/autorisation vérifiées
- [ ] Secrets non codés en dur
- [ ] Les messages d'erreur ne divulguent pas d'info

## Liste de vérification performance

- [ ] Pas de requêtes N+1
- [ ] Les requêtes de base de données sont indexées
- [ ] Les grandes listes sont paginées
- [ ] Les opérations coûteuses sont en cache
- [ ] Pas d'I/O bloquante dans les chemins critiques

## Liste de vérification test

- [ ] Chemin heureux testé
- [ ] Cas limites couverts
- [ ] Cas d'erreur testés
- [ ] Noms de tests descriptifs
- [ ] Les tests sont déterministes

Technique 2 : L'approche par questions

Au lieu de constater les problèmes, posez des questions pour encourager la réflexion :

❌ "Cela échouera si la liste est vide."
✅ "Que se passe-t-il si `items` est un tableau vide ?"

❌ "Tu as besoin de gestion d'erreur ici."
✅ "Comment cela devrait-il se comporter si l'appel API échoue ?"

❌ "C'est inefficace."
✅ "Je vois que cela boucle sur tous les utilisateurs. Avons-nous
envisagé l'impact de performance avec 100 000 utilisateurs ?"

Technique 3 : Suggérez, ne commandez pas

## Utilisez un langage collaboratif

❌ "Tu dois changer cela pour utiliser async/await"
✅ "Suggestion : async/await pourrait rendre cela plus lisible :
`typescript
    async function fetchUser(id: string) {
        const user = await db.query('SELECT * FROM users WHERE id = ?', id);
        return user;
    }
    `
Qu'en penses-tu ?"

❌ "Extrais cela dans une fonction"
✅ "Cette logique apparaît à 3 endroits. Aurait-il du sens de
l'extraire dans une fonction utilitaire partagée ?"

Technique 4 : Différenciez la gravité

Utilisez des étiquettes pour indiquer la priorité :

🔴 [blocking] - Doit être corrigé avant la fusion
🟡 [important] - Devrait être corrigé, discutez si désaccord
🟢 [nit] - Agréable à avoir, non bloquant
💡 [suggestion] - Approche alternative à considérer
📚 [learning] - Commentaire éducatif, aucune action requise
🎉 [praise] - Bon travail, continuez comme ça !

Exemple :
"🔴 [blocking] Cette requête SQL est vulnérable à l'injection.
Veuillez utiliser des requêtes paramétrées."

"🟢 [nit] Envisage de renommer `data` en `userData` pour plus de clarté."

"🎉 [praise] Excellente couverture de test ! Cela attrapera les cas limites."

Patterns spécifiques à un langage

Revue de code Python

# Vérifiez les problèmes spécifiques à Python

# ❌ Arguments par défaut mutables
def add_item(item, items=[]):  # Bug ! Partagé entre appels
    items.append(item)
    return items

# ✅ Utilisez None comme défaut
def add_item(item, items=None):
    if items is None:
        items = []
    items.append(item)
    return items

# ❌ Attraper trop large
try:
    result = risky_operation()
except:  # Attrape tout, même KeyboardInterrupt !
    pass

# ✅ Attraper les exceptions spécifiques
try:
    result = risky_operation()
except ValueError as e:
    logger.error(f"Invalid value: {e}")
    raise

# ❌ Utiliser des attributs de classe mutables
class User:
    permissions = []  # Partagé entre toutes les instances !

# ✅ Initialiser dans __init__
class User:
    def __init__(self):
        self.permissions = []

Revue de code TypeScript/JavaScript

// Vérifiez les problèmes spécifiques à TypeScript

// ❌ Utiliser any annule la sécurité des types
function processData(data: any) {  // Évitez any
    return data.value;
}

// ✅ Utilisez les types appropriés
interface DataPayload {
    value: string;
}
function processData(data: DataPayload) {
    return data.value;
}

// ❌ Ne pas gérer les erreurs asynchrones
async function fetchUser(id: string) {
    const response = await fetch(`/api/users/${id}`);
    return response.json();  // Et si la réseau échoue ?
}

// ✅ Gérez les erreurs correctement
async function fetchUser(id: string): Promise<User> {
    try {
        const response = await fetch(`/api/users/${id}`);
        if (!response.ok) {
            throw new Error(`HTTP ${response.status}`);
        }
        return await response.json();
    } catch (error) {
        console.error('Failed to fetch user:', error);
        throw error;
    }
}

// ❌ Mutation des props
function UserProfile({ user }: Props) {
    user.lastViewed = new Date();  // Mutation de prop !
    return <div>{user.name}</div>;
}

// ✅ Ne mutez pas les props
function UserProfile({ user, onView }: Props) {
    useEffect(() => {
        onView(user.id);  // Notifiez le parent pour mettre à jour
    }, [user.id]);
    return <div>{user.name}</div>;
}

Patterns de revue avancés

Pattern 1 : Revue architecturale

Lors de la révision de changements importants :

1. **Document de conception d'abord**
   - Pour les grandes fonctionnalités, demandez un doc de conception avant le code
   - Révisez la conception avec l'équipe avant l'implémentation
   - Acceptez l'approche pour éviter le rework

2. **Révisez par étapes**
   - Premier PR : Abstractions et interfaces principales
   - Deuxième PR : Implémentation
   - Troisième PR : Intégration et tests
   - Plus facile à réviser, plus rapide à itérer

3. **Considérez les alternatives**
   - "Avons-nous envisagé d'utiliser [pattern/library] ?"
   - "Quel est le compromis par rapport à l'approche plus simple ?"
   - "Comment cela évoluera-t-il quand les exigences changeront ?"

Pattern 2 : Revue de qualité des tests

// ❌ Mauvais test : Test des détails d'implémentation
test('increments counter variable', () => {
    const component = render(<Counter />);
    const button = component.getByRole('button');
    fireEvent.click(button);
    expect(component.state.counter).toBe(1);  // Test de l'état interne
});

// ✅ Bon test : Test du comportement
test('displays incremented count when clicked', () => {
    render(<Counter />);
    const button = screen.getByRole('button', { name: /increment/i });
    fireEvent.click(button);
    expect(screen.getByText('Count: 1')).toBeInTheDocument();
});

// Questions de révision pour les tests :
// - Les tests décrivent-ils le comportement, pas l'implémentation ?
// - Les noms de tests sont-ils clairs et descriptifs ?
// - Les tests couvrent-ils les cas limites ?
// - Les tests sont-ils indépendants (pas d'état partagé) ?
// - Les tests peuvent-ils s'exécuter dans n'importe quel ordre ?

Pattern 3 : Revue de sécurité

## Liste de vérification de sécurité

### Authentification et autorisation

- [ ] L'authentification est-elle requise si nécessaire ?
- [ ] Les vérifications d'autorisation sont-elles avant chaque action ?
- [ ] La validation JWT est-elle correcte (signature, expiration) ?
- [ ] Les clés API/secrets sont-ils correctement sécurisés ?

### Validation des entrées

- [ ] Toutes les entrées utilisateur validées ?
- [ ] Les uploads de fichiers sont-ils restreints (taille, type) ?
- [ ] Les requêtes SQL utilisent-elles la paramétrisation ?
- [ ] Protection XSS (échappement de sortie) ?

### Protection des données

- [ ] Les mots de passe sont-ils hashés (bcrypt/argon2) ?
- [ ] Les données sensibles sont-elles chiffrées au repos ?
- [ ] HTTPS est-il appliqué pour les données sensibles ?
- [ ] Les PII sont-elles gérées selon la réglementation ?

### Vulnérabilités courantes

- [ ] Pas de eval() ou exécution dynamique similaire ?
- [ ] Pas de secrets codés en dur ?
- [ ] Protection CSRF pour les opérations modifiant l'état ?
- [ ] Rate limiting sur les endpoints publics ?

Donner des retours difficiles

Pattern : La méthode du sandwich (modifiée)

Traditionnel : Éloge + Critique + Éloge (semble faux)

Mieux : Contexte + Problème spécifique + Solution utile

Exemple :
"J'ai remarqué que la logique de traitement des paiements
est en ligne dans le contrôleur. Cela rend plus difficile
la testabilité et la réutilisabilité.

[Problème spécifique]
La fonction calculateTotal() mélange le calcul de taxe,
la logique de remise et les requêtes de base de données,
rendant difficile le test unitaire et le raisonnement.

[Solution utile]
Pourrions-nous extraire cela dans une classe PaymentService ?
Cela le rendrait testable et réutilisable. Je peux faire
du pair avec toi si c'est utile."

Gérer les désaccords

Quand l'auteur est en désaccord avec votre retour :

1. **Cherchez à comprendre**
   "Aide-moi à comprendre ton approche. Qu'est-ce qui t'a mené
   à choisir ce pattern ?"

2. **Reconnaissez les points valides**
   "C'est un bon point sur X. Je n'avais pas envisagé cela."

3. **Fournissez des données**
   "Je suis préoccupé par la performance. Pouvons-nous ajouter
   un benchmark pour valider l'approche ?"

4. **Escaladez si nécessaire**
   "Faisons intervenir [architecte/développeur senior] pour peser sur cela."

5. **Sachez quand lâcher prise**
   Si cela fonctionne et que ce n'est pas un problème critique,
   approuvez-le. La perfection est l'ennemi du progrès.

Bonnes pratiques

1. Révisez rapidement : Dans les 24 heures, idéalement le même jour
2. Limitez la taille du PR : 200-400 lignes max pour une revue efficace
3. Révisez par blocs de temps : 60 minutes max, prenez des pauses
4. Utilisez des outils de revue : GitHub, GitLab, ou des outils dédiés
5. Automatisez ce que vous pouvez : Linters, formateurs, scans de sécurité
6. Construisez la relation : Les emojis, les éloges et l'empathie comptent
7. Soyez disponible : Offrez-vous pour faire du pair sur des problèmes complexes
8. Apprenez des autres : Lisez les commentaires de revue des autres

Pièges courants

• Perfectionnisme : Bloquer les PRs pour des préférences mineures de style
• Scope creep : "Pendant que tu y es, peux-tu aussi..."
• Incohérence : Des standards différents pour différentes personnes
• Revues retardées : Laisser les PRs traîner pendant des jours
• Ghosting : Demander des changements puis disparaître
• Approbation de routine : Approuver sans vraiment réviser
• Bike shedding : Débattre extensivement des détails triviaux

Templates

Template de commentaire de revue de PR

## Résumé

[Brève vue d'ensemble de ce qui a été révisé]

## Points forts

- [Ce qui a été bien fait]
- [Bons patterns ou approches]

## Changements requis

🔴 [Problème bloquant 1]
🔴 [Problème bloquant 2]

## Suggestions

💡 [Amélioration 1]
💡 [Amélioration 2]

## Questions

❓ [Clarification nécessaire sur X]
❓ [Considération d'approche alternative]

## Verdict

✅ Approuver après traitement des changements requis