emrevarol.com

Supports de Cours

Bonnes Pratiques de Programmation et Revue de Code Efficace

A2SV - University of Rwanda

Voir la Présentation

Liste de Vérification A2SV pour la Revue de Code

La référence absolue. Dix catégories pour évaluer chaque pull request que vous examinez.

1 Clarté et Lisibilité
Questions
  • Les noms de fonctions et de variables sont-ils clairs ?
  • Puis-je comprendre le code sans deviner ?
  • L'intention est-elle évidente ?
  • La logique est-elle facile à suivre ?
Signaux d'alerte
Des noms comme x, tmp, data, calc, process ; logique profondément imbriquée ; grandes fonctions peu claires
2 Qualité des Fonctions et de la Conception
Questions
  • Chaque fonction fait-elle une seule chose claire ?
  • La logique métier est-elle séparée des effets de bord ?
  • La conception est-elle facile à tester ?
  • Une fonction en fait-elle trop ?
Signaux d'alerte
Validation + sauvegarde + email + journalisation dans une seule fonction ; affichage dans les fonctions logiques ; comportement codé en dur réparti dans le fichier
3 Exactitude
Questions
  • Le code résout-il réellement le problème correctement ?
  • Les hypothèses sont-elles explicites ?
  • Tous les cas sont-ils traités ?
Signaux d'alerte
Validation d'entrée manquante ; hypothèses sur les clés de dictionnaire ; valeurs magiques ; échec silencieux
4 Cas Limites
Questions
  • Que se passe-t-il avec une entrée vide ?
  • Que se passe-t-il avec une entrée invalide ?
  • Que se passe-t-il avec des clés manquantes ?
  • Que se passe-t-il avec des valeurs nulles ou négatives ?
Signaux d'alerte
Seul le scénario nominal est considéré ; pas de vérifications défensives là où c'est nécessaire
5 Tests
Questions
  • Des tests sont-ils inclus ?
  • Les tests vérifient-ils des résultats exacts ?
  • Les cas limites sont-ils couverts ?
  • Ces tests détecteraient-ils des régressions ?
Signaux d'alerte
Un seul test ; assertions faibles comme > 0 ; pas de cas d'entrée invalide ; pas de tests pour la logique modifiée
6 Duplication
Questions
  • La logique est-elle répétée ?
  • La logique partagée peut-elle être extraite ?
  • Les règles métier sont-elles dupliquées à plusieurs endroits ?
Signaux d'alerte
Formules répétées ; blocs de conditions répétés ; valeurs codées en dur répétées
7 Gestion des Erreurs
Questions
  • Les erreurs sont-elles gérées clairement ?
  • Le code échoue-t-il de manière compréhensible ?
  • Les états invalides sont-ils visibles ?
Signaux d'alerte
Retourner None sans explication ; exceptions avalées ; modes d'échec cachés
8 Maintenabilité
Questions
  • Sera-t-il facile de l'étendre plus tard ?
  • Un autre ingénieur pourra-t-il le modifier en toute sécurité ?
  • Cela rend-il la base de code plus propre ou plus désordonnée ?
Signaux d'alerte
Règles codées en dur ; PR trop volumineuse ; logique fortement couplée
9 Qualité du Dépôt
Questions
  • La structure du dépôt est-elle claire ?
  • Le README est-il utile ?
  • Un nouvel ingénieur peut-il lancer le projet facilement ?
  • Les dépendances sont-elles visibles ?
Signaux d'alerte
Pas d'instructions d'installation ; README faible ; organisation confuse ; répertoire de tests manquant
10 Qualité de la PR
Questions
  • La PR est-elle ciblée ?
  • Le titre est-il descriptif ?
  • La description est-elle utile ?
  • La taille de la modification est-elle raisonnable ?
Signaux d'alerte
Titres vagues ; pas d'explication ; modifications non liées mélangées ; PR gigantesques

Modèles de Commentaires de Revue

Formulations professionnelles pour les scénarios de revue courants. Utilisez-les comme point de départ.

Nommage
« Pourrait-on renommer cette fonction pour mieux refléter son objectif ? »
« Ce nom de variable semble trop générique. Peut-on le rendre plus descriptif ? »
Logique
« Pourrait-on simplifier cette condition pour améliorer la lisibilité ? »
« Serait-il pertinent d'extraire cette logique dans une fonction auxiliaire ? »
Tests
« Peut-on ajouter un test pour le cas d'entrée vide ? »
« Pourrait-on vérifier la valeur exacte attendue ici au lieu d'utiliser une vérification large ? »
Structure
« Cette fonction semble faire plusieurs choses. Peut-on séparer les responsabilités ? »
« Découper cela en fonctions plus petites faciliterait-il les tests et la revue ? »
Cas Limites
« Que se passe-t-il si cette clé est manquante ? »
« Devrait-on valider l'entrée avant de la traiter ? »
Qualité de la PR
« Pourriez-vous ajouter une courte description de PR expliquant la modification et comment elle devrait être testée ? »

Démo en Direct : Examinez cette Pull Request

Une PR délibérément défectueuse pour s'entraîner. Pouvez-vous repérer les problèmes ?

Branche : feature/cart-total-update
Titre : "Update cart calculation logic"
Description : "Updated the cart calculation logic and added support for discount."
src/cart.py
def calc(items, user, d=False): t = 0 for i in items: if i["type"] == "book": t += i["price"] * 0.9 else: t += i["price"] if d: t = t - t * 0.1 if user is not None: if "is_premium" in user: if user["is_premium"] == True: t = t - t * 0.05 return t def process_cart(items, user): total = calc(items, user, True) print("cart processed") return total
tests/test_cart.py
from src.cart import calc def test_calc(): items = [{"type": "book", "price": 100}] user = {"is_premium": True} assert calc(items, user, True) > 0
README.md
# Cart Project cart logic
Guide de Revue - Problèmes à Trouver
Problèmes au niveau de la PR
  • Titre de PR vague : "Update cart calculation logic" ne décrit pas ce qui a changé
  • La description est générique et inutile
  • Pas de plan de test ni d'instructions pour les relecteurs
Problèmes au niveau du code
  • Le nom de fonction calc est trop générique - devrait être calculate_cart_total
  • Le paramètre d est peu clair - devrait être discount_enabled
  • Les variables t et i ne sont pas descriptives
  • Nombres magiques : 0.9, 0.1, 0.05 devraient être des constantes nommées
  • Blocs if profondément imbriqués pour la vérification premium - utiliser .get()
  • process_cart a un effet de bord (print) mélangé avec la logique métier
  • Pas de validation d'entrée pour items ou user
  • Le paramètre booléen d=False passé positionnellement comme True est confus
Problèmes au niveau des tests
  • Un seul cas de test pour l'ensemble du module
  • Assertion faible : > 0 ne vérifie pas l'exactitude
  • Pas de tests de cas limites (liste vide, clés manquantes, utilisateur non premium)
  • Ne teste pas process_cart
Problèmes au niveau du dépôt
  • Le README est essentiellement vide - pas d'instructions d'installation, pas de description
  • Aucune information sur comment lancer les tests
Version Améliorée - cart_improved.py
src/cart_improved.py
BOOK_DISCOUNT_RATE = 0.10 GENERAL_DISCOUNT_RATE = 0.10 PREMIUM_DISCOUNT_RATE = 0.05 def calculate_item_price(item): item_type = item["type"] price = item["price"] if item_type == "book": return price * (1 - BOOK_DISCOUNT_RATE) return price def apply_general_discount(total_price, discount_enabled): if not discount_enabled: return total_price return total_price * (1 - GENERAL_DISCOUNT_RATE) def apply_premium_discount(total_price, user): if not user or not user.get("is_premium", False): return total_price return total_price * (1 - PREMIUM_DISCOUNT_RATE) def calculate_cart_total(items, user, discount_enabled=False): total_price = 0 for item in items: total_price += calculate_item_price(item) total_price = apply_general_discount(total_price, discount_enabled) total_price = apply_premium_discount(total_price, user) return total_price def process_cart(items, user): total_price = calculate_cart_total(items, user, discount_enabled=True) return total_price

Devoir

Exercice de Revue de PR : Relisez comme un Ingénieur Professionnel

Votre soumission doit inclure :

  • 10 au total Minimum de commentaires de revue
  • 3 min Commentaires liés au nommage
  • 2 min Commentaires liés aux tests
  • 2 min Commentaires sur la conception ou la responsabilité des fonctions
  • 1 min Commentaire sur la qualité du dépôt
  • 1 min Suggestion de refactoring

Format de Soumission

File: src/orders.py
Line/Area: calc function
Comment: The function name is too generic. Could we rename
it to calculate_order_total for clarity?

File: tests/test_orders.py
Line/Area: test_calc
Comment: This test only checks that the result is greater
than zero. Could we assert the exact expected value instead?

Examinez les fichiers suivants :

src/orders.py
def calc(items, vip=False): total = 0 for i in items: if i["type"] == "book": total += i["price"] * 0.9 elif i["type"] == "electronics": total += i["price"] else: total += i["price"] if vip == True: total = total - total * 0.05 return total def checkout(items, user): total = calc(items, user["vip"]) print("checking out order...") print("total is", total) return {"ok": True, "total": total}
src/utils.py
def f(x): if x == None: return False return True
tests/test_orders.py
from src.orders import calc def test_calc(): items = [{"type": "book", "price": 100}] assert calc(items, False) > 0
README.md
# Orders Run it with python.
Les grands ingénieurs ne se contentent pas de faire fonctionner le code. Ils rendent le code compréhensible, testable, révisable et fiable.
Citation du Jour
« Le code que vous êtes trop occupé pour nettoyer aujourd'hui devient le problème qui accapare votre équipe demain. »