Réaliser des revues de code est une activité que je pratique régulièrement sur les projets que j’encadre techniquement. De manière général, elles se déroulent sur le poste du développeur. Ce dernier me présente ses derniers changements, justifie ses choix et m’explique ses difficultés. En fonction de son expérience sur le projet, la périodicité des revues varie d’1 fois par jour à 2 fois par mois.
Les améliorations à apporter sont réalisées en séance en pair programming ou bien consignées directement dans le code à l’aide d’un TODO. Je profite de ces moments privilégiés pour expliquer et/ou échanger autour des règles de coding et d’architecture logicielle.
Dans le cadre de l’externalisation du développement d’une application, les revues de code se pratiquent différemment. En effet, la livraison du code intervient souvent après un long effet tunnel. Dès le début des développements, les développeurs doivent connaître mes attentes. Ce billet a pour objectif de les formaliser de manière synthétique.
Sonar, bien mais pas suffisant
Dans ces 2 types de revues, la qualité du code est mesurée automatiquement par l’outil SonarQube. Des seuils sont fixés pour chaque métrique : taux de couverture de tests, nombre de défauts … Accessible en ligne et documenté, le profil SonarQube fait office de référence sur les règles à respecter : de la simple règle de formatage du code à la gestion des exceptions.
Pour autant, toutes les bonnes pratiques en termes de qualité du code et d’architecture ne peuvent pas être contrôlées par cet outil. Qui plus est, il est parfois possible de leurrer SonarQube. Par exemple, en utilisant des outils de génération automatique de tests unitaires (TU) ou en développement des TU sans assertions, on augmente artificiellement la couverture de code. Une revue « manuelle » reste donc obligatoire.
Les points qui sont surveillés lors de cette revue sont référencés dans une check-list. Ces points ont été répartis en 5 catégories :
- Code Design
- Architecture logicielle
- Performance
- Tests
- Sécurité
Certains points sont spécifiques à l’architecture technique de l’application. Dans notre exemple, l’application web est décomposée en 3 couches et repose sur les technologies Spring MVC, CXF et JDBC.
Sans surprise, de nombreuses de règles sont tirées de l’ouvrage de référence des développeurs « Clean Code » d’Uncle Bobo. Les autres proviennent de bonnes pratiques, d’état de l’art et d’expériences.
Code Design
| Rubrique | Description |
| Qualité | Respecter les règles du profil « Sonar way » de SonarQube |
| Lisibilité du code | Code élégant et facile à lire. Les classes et les méthodes doivent être relativement petites (classes < 300 lignes et méthodes < 20). Les conditions doivent être compréhensibles. Homogénéité du code : règles de nommage, découpage en package … Pas de code mort ni de code commenté. Noms appropriés (doit révéler l’intention, ne pas les tronquer ou le abréger) |
| Simplicité | Principe KISS. Pas d’over-design Utilisation de Design Pattern justifiée Dépendances vers d’autres classes minimales |
| Réutilisation | Framework open source à privilégier sur du code maison. Code factorisé. Code dupliqué à éviter. Utilisation appropriée de l’héritage et de la composition |
| Commentaires | La javadoc apporte une plus-value et doit aider à la maintenance de l’application. Interfaces, Classes, propriétés et méthodes publiques doivent comporter une Javadoc. Les commentaires dans le code doivent se limiter à des explications (le pourquoi). |
| Configuration | Les variables susceptibles de changer d’un environnement à l’autre doivent être externalisées et variabilisées par environnement. Le texte et les libellés des pages JSP sont externalisés dans des message bundles (.properties) |
Architecture logicielle
| Rubrique | Description |
| Gestion des transactions | Les transactions base de données sont gérées au niveau de la couche des services métier. |
| Configuration Spring | Privilégier les annotations Spring à la configuration XML (moins verbeux). La configuration XML ou Java est réservée aux beans d’infrastructure et à la configuration de l’architecture applicative. |
| Découplage | Utiliser l’inversion de dépendances (Spring IoC). Utiliser des types abstraits ou des interfaces. Eviter les appels de méthodes statiques. |
| Thread-safe | Les ressources partagées entre 2 requêtes HTTP doivent être synchronisées. Attention aux beans Spring de portées singleton et prototype avec états. |
| Gestion des exceptions | Utiliser des exceptions vérifiées pour les erreurs fonctionnelles récupérables. Utiliser les exceptions non vérifiées (RuntimeException) pour les erreurs techniques non récupérables. Lever des exceptions dès que nécessaire (au plus tôt). Programmation défensive, par assertions. Traiter les exceptions au niveau le plus haut. Traiter les exceptions dans les niveaux intermédiaires que si nécessaire. |
| Frameworks | Utiliser les bibliothèques et frameworks référencés dans le catalogue des normes et standards l’EntrepriseL’ajout de dépendances tierces est soumise à dérogation et se devra d’être justifié. |
| Singleton | Ne pas utiliser le pattern Singleton. Laisser Spring gérer le cycle de vie des objets (beans de portée singleton) |
| Logs applicatifs | Messages de logs pertinents et contextualisés. L’encapsulation d’une exception apporte des informations complémentaires sur le contexte d’appel. Login de l’utilisateur affiché systématiquement grâce au MDC de SLF4J.Ne pas logger 2x la même erreur.Seules les erreurs techniques sont loggés avec le ERROR. |
| Découpage en couches | Respect du découpage en 3 couches : Contrôleur => Services métiers => DAO/Repository. Les services métiers et les DAO sont déclarés dans le contexte root. Les contrôleurs Spring MVC sont quant à eux déclarés dans un contexte enfant. |
Performances
| Rubrique | Description |
| Web Service | Limiter le nombre d’appel de WS.Echouer rapidement (timeout faible). |
| Base de données | Maitriser le nombre de requêtes SQL exécutés par un DAO. Utiliser des index. Eviter les recherches de type like commençant par un %.Lorsque la pagination n’est pas utilisée, toujours limiter le nombre de résultats remontés par une requête. |
| Cache | L’utilisation du cache Hibernate ou d’un cache applicatif doit être motivée. Les gains doivent pouvoir être mesurés. L’application doit fonctionner lorsque le cache est désactivé. |
| Mapping Objet-Objet | Les mappings entre DTO doivent être réalisés manuellement en Java. L’utilisation de frameworks comme Dozer ou Orika est proscrite. |
Tests unitaires
| Rubrique | Description |
| Règles d’or | Aussi important que du code de production. Permet de documenter le code.Utiliser des noms de méthode qui documentent le TU.Un scénario par méthode de test. |
| Périmètre | Tester les cas limites. Tester les exceptions. Tester les requêtes SQL. Un test sans assertion ne vaut (presque) rien. |
| DAO / Repository | DbUnit (ou DbSetup) et une base de données embarquées peuvent être utilisée pour tester les DAO. |
Sécurité
| Rubrique | Description |
| SQL | Utiliser des PreparedStatement avec JDBC. |
| Logs | Ne pas logger des données sensibles. |
| Web | Les applications web sont sécurisées avec Spring Security. Valider systématiquement les données saisies par l’utilisateur. Un utilisateur ne doit pas pouvoir escalader ses propres privilèges en forgeant sa propre requête HTTP. |
Conclusion
Cette check-list n’est sans doute pas exhaustive. Qui plus est, vous ne serez sans doute pas d’accord avec tous ces principes. Libre à vous de la personnaliser en fonction de vos contraintes techniques et de vos règles d’architecture. N’hésitez pas non plus à laisser vos commentaires afin d’en débattre. En fonction de vos retours, je complèterai/amenderai ce billet.
Salut Antoine,
Le monde de l’IT est petit. Je suis tombé par hasard sur cet article et après vérification de la photo et du profil LinkedIn, c’est bien toi ! Petit moment de nostalgie avec une belle aventure partagée sur Target2… Merci pour cet article qui résume de manière pragmatique la revue de code. Je mets dans mes favoris ! Bonne continuation…
Bonjour Thomas, quelle belle surprise. Le monde de l’informatique est décidément petit, même éloigné de Paris 😉
Neuf ans ont passés depuis ce beau projet qu’est Target 2. Cela reste sans doute ma plus belle référence et j’y ai rencontré de très bons développeurs (même si certains font aujourd’hui moins de technique ;-).
En tout cas, cela me fait plaisir de te savoir un de mes lecteurs 😉 A bientôt sur ce blog ou ailleurs.
Merci pour cet article intéressant.
Un point m’interpelle : pourquoi proscrire les frameworks de mapping (dozer, orika, …) ?
Nous sommes justement en train de nous interroger sur le sujet et un retour d’expérience nous serait profitable.
Concrètement, aujourd’hui, on se retrouve dans une de nos applications avec des POJOs annotés pour hibernate + JAXB + Jackson, ce qui impliquent des contraintes fortes et nous envisagions de séparer les entités dédiées à hibernate de celles dédiées à la sérialisation XML ou JSON avec du mapping entre les 2. On se demande aussi si on ne devrait pas partir systématiquement d’un modèle métier central pour mapper vers ces nouveaux POJOs. Le mapping manuel étant loin d’être la panacée, un framework de mapping nous semblait profitable.
C’est pourquoi je suis curieux d’en savoir plus sur les raisons qui pourraient amener à proscrire un tel framework.
Si c’est pour des raisons de performance (c’est dans cette rubrique que se trouve cette préconisation) plus que d’architecture, n’existe-t-il pas aujourd’hui de frameworks de mapping qui procurent des performances à la hauteur d’un mapping manuel ? Je n’ai aucune pratique de ces frameworks mais j’ai cru comprendre que les plus récents d’entre eux (selma, modelmapper, … ?) tentaient de répondre à ces impératifs.
Bonjour Laurent,
D’une manière générale, j’aurais tendance à éviter le mapping objet / objet. Outre les problèmes de performance évoqués, cette couche de mapping complexifie l’architecture applicative et peut être source de bugs. Faire transiter une entité métier de la couche de persistance aux IHM reste le plus simple.
Cela dit, dans certains cas, le mapping se révèle indispensable. Je pense par exemple à l’exposition de web services SOAP versionnés. Dans votre application où vos POJOs portent à la fois des annotations JPA, JAXB et Jackson, votre questionnement est tout à fait légitime. Un tableau pondéré des avantages / inconvénients pourrait vous aider à faire le meilleur choix. Dans un prochain billet, je vais tâcher de retranscrire le notre. A l’époque, nous n’avions étudié ni Selma ni Modelmapper. Mais j’y mettrai le résultat des micro-benchmarks réalisés avec Dozer et Orika.
Stay tuned.
Antoine
Laurent, comme promis, voici un article faisant à la fois un retour d’expérience de l’utilisation de Dozer, mais comparant également les performances de 5 frameworks de mapping objets : Selma, Dozer, Orika, ModelMapper et MapStruct
https://javaetmoi.com/2015/09/benchmark-frameworks-java-mapping-objet/
Bonjour,
Je suis ingénieur d’étude et développement, 3 ans d’expérience en dev j2ee.
J’aimerais savoir si possible quel est le poste que vous occupé et quelles sont vos tâches.
En fait, j’aimerai prendre un poste de revue de code.
Merci d’avance
Bonsoir,
Je ne sais pas s’il existe un poste de « reviewer de code ». Mais rassurez-vous : les revues peuvent être menées à différents postes : développeur, tech lead, équipe support transverse … En tant qu’ingénieur d’études Java, vous pouvez proposer de mettre en place sur votre projet des revues de pair ou des revues de code.
Pour ma part, j’occupe actuellement un poste de référent technique et les revues occupent au maximum 5% de mon temps.
Antoine