La Code Review c’est bien mais encore faut-il savoir quoi reviewer…​

Cette liste est très très loin d’être exhaustive. Elle est issue d’une expérience concrète sur un précédent projet (principalement Java, SpringBoot, React). Elle n’est pas forcément applicable "telle qu’elle" dans tous les contextes et pour tous les projets mais fournit de bons premiers éléments.

Rester ouvert d’esprit et agile.
Il faut garder à l’esprit de ne pas appliquer tous les points de manière trop stricte : dans certains cas, certaines règles peuvent être reconsidérées si le besoin se justifie.
Toute la difficulté du métier de développeur est de savoir prendre du recul, de savoir appliquer les bonnes règles selon le contexte et parfois s’autoriser des exceptions.

Préalable à la revue de code

Avant même de faire la revue de code, se poser les questions suivantes :

  • Est-ce que l’implémentation répond au besoin énoncé par l’US?

  • Est-ce que la feature fonctionne correctement dans le navigateur cible ?

  • Est-ce que les R.G. sont toutes implémentées ?

  • Est-ce que les Test d’Acceptance de l’US sont OK ?

  • Est-ce que ça build ? (via mvn compile ou autre)

  • Est-ce que les TU/TI passent à 100% ?

  • Est-ce que le code est bien à jour par rapport à develop (rebase)?

Si une des réponses est NON, alors, pas besoin d’aller plus loin !!

Quels outils pour m’aider dans ma Code Review?

  • SonarQube

  • Le plugin IntelliJ GitScope pour voir le diff entre develop et une feature-branch au sein de l’IDE

  • Le plugin VSCode GitLens pour voir le diff entre develop et une feature-branch au sein de l’IDE

  • Le plugin IntelliJ SonarLint pour voir les anomalies sur les fichiers, directement depuis l’IDE

  • Le ChangeSet de la Merge Request Gitlab, les Approvals.

Quelques concepts à garder à l’esprit :

  • Le Rasoir d’Ockham n’est pas le titre d’un album de Tintin

  • KISS n’est pas juste qu’un groupe de rock kitch avec un maquillage improbable.

  • Non, le NIH n’est pas la dernière MST que les ado se refilent.

  • YAGNI n’est pas une danse de Fortnite

  • DRY n’est pas applicable qu’au Gin ! (alcoolo, va !)

  • Pas besoin de faire de muscu pour être SOLID

Revue de Code - Généralités

Je ne reviens pas sur les conventions générales des langages (qui sont déjà disponibles ailleurs), mais plutôt les spécificités et l’architecture logicielle du code source.

Enlever le superflu

  • Pas des sources superflues (fichiers temporaires, fichiers de constructions, binaires finaux, fichiers de logs)

  • Pas de fichiers spécifiques aux développeurs. (fichiers d’IDE par exemple - .settings, .projects). Le code source versionné doit rester indépendant de l’IDE.

  • Pas de fichiers historiques (genre les fichiers nommés *-old.xx ou -sauv ou -backup)

  • pas de portions de code mort. Les portions de code en commentaires n’ont pas d’intérêt à rester : créer une branche dédiée si besoin ou le placer dans des snippets, etc. Mais certainement pas dans develop.

Organiser correctement

  • Est-ce que les modules fonctionnels (domaines) sont correctement nommés ? Explicites et parlant ? Est-ce qu’ils suivent les nommages métiers partagés (au sens DDD)?

  • Est-ce qu’il n’y a pas de duplication de code (module déjà existant, classe utilitaire ou fonctions existantes)

  • Est-ce la fonction est bien placée là où elle devrait ?

Modulariser des sources

  • Est-ce que la fonctionnalité suit sur le découpage en domaine ?

  • Est-ce que le découpage est d’abord fonctionnel, puis technique ?

  • Est ce que les couches techniques repository, domain, utils, common, business sont présentes et correctement nommées ?

  • Est ce qu’il n’est pas sur-découpé ? Eviter une trop grande profondeur de niveaux fonctionnels ou techniques

  • Est-ce que l’on privilégie bien la composition plutot que l’héritage ?

  • Est ce que les classes suivent la convention de nommage de package interne : fr.mycompany.myawesomeproject.domain.xxxxx ?

Bien nommer les choses

Se reférrer au post précédent sur les nommages

  • Les termes métiers sont en français.

  • Les briques techniques sont en anglais (Utiltaires, etc..).

  • Est-ce que les Utilitaires sont dans un module core ou common ?

  • Est-ce que les nommages sont cohérents et respectent le contract (ils font ce qu’ils disent qu’il font ?) par exemple : un readXX est idempotent et ne modifie pas l’état d’un objet.

  • Est ce que les variables d’ID sont nommées de manière cohérente dans l’application (par exemple s’il y’a un idProject, on devrait avoir un idUser et non pas un userId)?

  • Nommage des fichiers : Est-ce que les fichiers sont bien nommés selon le langage ou la technologie?

  • Nommage des classes : Est-ce que les classes sont bien nommées en adéquation avec le nom du fichier ?

Rappel sur les différents types de casses :

  • snake_case : mots en minuscule séparés par _ : ma_variable

  • camelCase : premier mot en minuscule, chaque autre commence par une majuscule : maVariable

  • PascalCase (aka UpperCamelCase): Chaque mot commence par une majuscule : MaClasse

  • kebab-case (aka spinal-case): mots en minuscule, séparés par -: ma-variable

Documenter ni trop ni trop peu

  • Privilégier l’autodocumentation :nommage explicite des méthodes, des paramétres qui parlent d’eux-mêmes.

  • Documentation présente pour expliquer l’objectif d’une fonction

    • Si algo "complexe", "déroutant", "pas trivial" : y a-t-il un commentaire pour expliquer son fonctionnement ?

    • Si une règle métier explique une portion de code particulière : l’indiquer sous forme de commentaire. Indiquer la RG ou la Mantis

  • Correction temporaire, patch "moche, mais nécessaire", code "laid, mais y a pas moyen de faire autrement" : indiquer pourquoi c’est fait comme ça dans un commentaire !

  • Eviter la sur-documentation : les commentaires sur les constructeurs par défauts, les getters/setters ou les méthodes héritées (hashCode, toString) n’ont aucun intérêt.

just because
Figure 1. courtesy of Geek & Poke

Maitriser les dépendances

  • la librairie est conforme aux préco de la Stack logicielle et validée au sein de la DSI ?

  • la version est à jour et compatible avec le framework actuel ?

  • la librairie (ou une version particulière de la librairie) est assurée de ne pas avoir de faille de sécurité ?

  • Est-ce que la licence de la librairie est compatible avec son utilisation sur le projet ?

  • Est-ce que les dépendances transitives n’entrent pas en conflit avec celles déjà tirées. Pensez aux exclusions pour ces cas !!

  • Est ce que les scope Maven sont correctement utilisés ? Scope test sur les librairies utilisées exclusivment pour les tests, etc.

Factoriser et réutiliser l’existant

  • y-a-t-il une création "maison" de brique techniques (ou de fonctions utilitaires) qui auraient pu être déléguées à une librairie existante ?

  • Est-ce qu’une fonction utilitaire n’existe pas déjà dans une librairie (apache Common) sous un autre nom ?

  • Utilisation de code "utilitaire", type Lombok, pour simplifier les développements ?

Lors des appels d’API, webservices et autres briques transverses "maison".

  • S’appuyer au maximum sur le SDK s’il est disponible. (Plutôt que recoder une nouvelle brique d’appel)

  • Privilégier les outils de générations (xml2pojo, json2pojo, etc.) pour générer le code, éviter les erreurs et gagner en temps et en maintenance!

  • Est-ce que la génération de code est portée par les outils lors de la phase de précompilation ?

Tester

  • Est-ce que des tests unitaires sont mis en place ?

  • Est-ce que la couverture sur les nouveaux tests est satisfaisante et en conformité avec les attendus projet ?

Gardez les choses simples !

Le Rasoir d’Ockham nous dit que la solution la plus simple est souvent la meilleure.

  • Est-ce que l’implémentation n’est pas overly-complex par rapport au besoin ?

  • N’y aurait-il pas eu une solution plus simple/lisible/compréhensible de l’implémenter ?

  • Over-engineering : Est-ce que l’implémentation reste bien dans le cadre du besoin de l’US ? Eviter les "j’ai ajouté ça en plus si on a besoin de faire ça plus tard…​". En général, les justifications avec des "si" sont douteuses.

Date, Temps et Fuseau

  • Est-ce que la Date est Universelle ou Locale ?

  • Est-ce que le fuseau de la date est bien conforme à l’attendu ?

  • Est-ce que le Test Unitaire est bien exécutable en dehors du temps présent (utilisation d’une Clock dédié au test pour assurer sa reproductibilité dans le contexte du test)

En vrac

  • Est-ce que le code contient des valeurs magiques ? (des valeurs de variables en dur dans le code, sans explication…​)

  • Est-ce que les paramètres d’entrées sont vérifiés avant traitement (nullité et validité). Que ce passe t’il si les valeurs sont hors-bornes ? Feature-Flipping : Est-ce que le code prend en compte la notion de feature-flipping (si exigée pour le domaine) ?

  • Trop de ternaire tue le ternaire. Ne pas imbriquer des ternaires : le code devient illisible

  • Supprimer les constructeurs par défaut (sauf quand ils sont explicitement utiles)

  • Est-ce que des logs sont correctement mis en place ? Est-ce que leur niveau est adapté ?

  • Est-ce que les variables dépendant des environnements ont été externalisés ?

Code Review : Java/Spring

Généralités Spring

  • Est-ce que l’injection de composant est correctement faite avec Spring ?

  • Est-ce que l’injection est faite prioritairement par constructeur ?

  • Est-ce que les beans sont correctement annotés (@Component, @Service, @Repository, etc.) ?

  • Est-ce qu’il n’y a pas d’ambiguité sur les injections de beans ? (nommages correct ?)

  • Est-ce qu’il n’y a pas de surdécoupage des beans (Interface/Implémentation)

Couche Données: Objets Métiers

  • Est-ce que les objets métiers utilisent les HashCode/Equals ?

  • Sont ils implémentés correctement ? ils doivent suivre une règle d’implémentation précise.

  • Est-ce que le equals() est implémenté correctement ? Le mieux est de s’appuyer sur les fonctions utilitaires ou les générer par l’IDE, c’est moins risque d’erreur.

  • Est-ce que les Repository sont bien stateless ?

  • Est-ce que les Repository se limitent bien à de la récupération d’objets métiers ?les traitement métiers sont réalisés dans les classes Service, pas dans les Repository

  • Est-ce que les getXXX() sur collection mettent en place un système de pagination pour éviter de retourner la base entière ?

  • L’ouverture de transaction ne doit pas se faire sur la couche Repository.Lors de l’appel au Repository, la transaction doit déjà être ouverte. l’annotation @Transactional ne doit pas être présente dans le Repository

Service et traitement métier

  • Est-ce que les contrôles d’accès sont correctement réalisés ?

  • Est-ce que la transaction est bien ouverte explicitement ?

  • Est-ce que le process métier est entièrement transactionnel ?

  • Est-ce que les données (et le système) sont toujours cohérentes si le process métier est annulé (ou en erreur) et que la transaction est rollbackée ?

  • Le service ne doit pas manipuler de DTO Metier (seul les DTO 'techniques' - objets de recherches, etc…​ sont autorisés)

  • Est-ce que chaque classe Service est bien stateless ?

  • Est-ce que chaque classe Service ne se limite bien qu’à du code métier ? (

  • Est-ce qu’il existe des variables de classes ? (alors qu’il ne devrait pas?)

Code Revue : API et Frontend

Controller et API REST

  • Es-ce que le niveau de maturité de l’API REST est conforme à ce qui est attendu ?

  • Est-ce que chaque URL est correctement constituée et leur verbe correctement implémenté ?

  • Est ce que les objets de retour de Controller sont bien des DTO ?

  • Est ce qu’un mécanisme de pagination est mis en place sur les retours de collections

  • Est ce qu’un mécanisme de réponse partielle est mis en place ?

  • Est-ce que le code retour correspond bien au verbe HTTP (par exemple un PATCH qui retourne un 200 au lieu de 204)

  • Les Entités métier ne devraient pas être manipulés dans la couche controller, seule leur représentation déshydratées (DTO ou Json)

  • Aucune ouverture de Transaction ne doit être faite dans cette couche

  • Est-ce que les paramètres d’entrées sont bien vérifiés avant l’appel à la couche service ?

  • Est-ce que la quantité de donnée transitant par l’API a été testée et validée ? Les données métiers sont-elles suffisamment déshydratées ?

Frontend

  • Est-ce que le champ est limité, lors de la saisie en taille ?

  • Est-ce que le champ attend une taille minimale ?

  • Est-ce que le champ autorise tous les caractères ( Que Alpha ou que numérique) ?

  • Est-ce que le champ autorise les caractères hors Latin-1 ? Caractères étrangers (tels que さくら), symboles mathématiques (tels que αελ), …​

  • Est-ce que le champ autorise des smileys , des drapeaux et autres emojis ?

  • Est-ce que le champ répond au format attendu (mail, SIRET, numéro de tel) ?

  • Refresh-potent : Un Refresh (F5) sur une page doit réafficher le même écran à chaque fois

  • Est-ce que le fichier d’internationalisation a été mise en place ?

  • CSS : Eviter les !important en CSS

  • Limiter l’utilisation des inline style. À mettre exceptionnellement quand il n’y a pas d’autres solutions

etc., etc.

Cette liste est évidemment loin d’être exhaustive ! Elle n’est qu’un rappel rapide des bonnes pratiques que l’on retrouve communément.

Cette liste doit devenir automatique et s’étoffer au fur et à mesure de l’expérience sur le projet.

Bons développements !