La pratique de la revue de code est répandue chez Epiconcept. Cet article à pour but d’homogénéiser les pratiques des différentes équipes sur le sujet.
Les guidelines ci-dessous sont issues de réunions. Elles n’ont pas l’ambition d’être des vérités absolues. Leur objectif est simplement de s’assurer qu’une même pratique sera acceptée ou refusée de la même manière dans toutes les équipes lors de la revue de code.
Guidelines sur les commentaires
Les PHPDoc de fichiers, classes et méthodes
On parle de ça par exemple :
Ces commentaires servent à l’autocomplétion dans les IDE.
Tip: Ces commentaires seront acceptés et même conseillé en revue de code.
Les commentaires dans le code
On parle de ça par exemple :
Important: Il peut y avoir des commentaires dans le code, mais il doivent être succints, utiles et compréhensibles.
Le code commenté
On parle de ça par exemple :
Soit le code est utile et doit être décommenté, soit il ne l’est pas et il doit être supprimé. Le laisser commenté laisse planer un doute. Et de plus, on a un outil de gestion de version pour gérer les suppressions de code et leur revert si nécessaire.
Warning: Le code commenté sera refusé en revue de code.
Guidelines sur les errorlog
On parle de ça :
On distingue trois types de logs :
Les logs d’erreurs : issus du catch d’une exception par exemple
Les logs d’activités : plus métiers (envoi d’un mail, appel à un service externe, etc.)
Les logs de debug : utiles pendant le développement mais pas sur les serveurs
Les logs d’erreur
Tip: Ces logs seront acceptés en revue de code.
Important: Attention cependant, ils doivent contenir suffisamment de contexte pour être utilisables (au minimum, le message de l’exception).
Exemple :
Les logs d’activités
Tip: Ces logs seront acceptés en revue de code, et même conseillés.
Important: Attention cependant, ils doivent écrire dans un fichier de destination dédié pour les distinguer des autres : space/log/[vhost de mon application]/activity/[mafonctionnalite].log. Ce chemin est disponible via la directive ini voozanoo.activity.log.directory.
Exemples :
Les logs de debug
Warning: Ces logs n’ont pas vocation à être sur les serveurs. Ils seront refusés en revue de code.
Guidelines sur les @todo
On parle de ça :
Important: Globalement, les @todo seront refusés en revue de code.
En effet :
Si le @todo n’est pas compréhensible, il faut le retirer.
Si le @todo est dans le périmètre du ticket, il faut faire le développement avant de faire la PR (car laisser un @todo voudrait dire que le travail n’est pas terminé).
Si le @todo est hors du périmètre du ticket, il faut créer un ticket autre Jira pour que ce soit planifié.
Guidelines sur l’usage des constantes
On parle de ça :
Elles permettent d’éviter de répéter une même valeur plein de fois dans le code (risque d’erreur et manque de factorisation).
Il n’y a pas de guidelines claires sur la préférence à avoir entre des constantes comme celles-ci et l’utilisation de propriétés de classe, mais quoi qu’il en soit :
Tip: Les constantes seront globalement acceptées en revue de code.
Warning: Les chaînes en dur un peu partout dans le code seront refusées en revue de code.
Guidelines sur les méthodes statiques
On parle de ça :
Ces méthodes permettent de ne pas instancier leur classe, mais sont souvent un raccourci vers de la conception “moins” orientée objet. Elles peuvent être justifiées par des questions de performances ou de lisibilité du code, mais ne sont globalement pas recommandées.
Plus précisement :
Warning: Les grosses classes ‘Helper’ contenant une collection de fonctions qui n’ont rien à faire ensemble seront refusées en revue de code (mauvais exemple ici).
Important: Au minimum, il faut que les fonctions regroupées ainsi soient peu nombreuses et traitent toutes du même sujet.
Guidelines sur l’usage des classes de Zend pour l’accès aux données
On parle de ça :
Cette façon de procéder, que ce soit pour un INSERT, un UPDATE, ou autre, est globalement interdite.
En effet, contrairement aux méthodes du noyau comme SimpleUpdateData(), elle présente deux problèmes :
les droits de l’utilisateur courant ne sont pas appliqués ;
les actions faites de cette manière ne sont pas tracées dans les tables evtlog et varsetmonitor de Voozanoo 4.
Or, ces deux points sont des engagements que nous avons vis-à-vis de nos clients et des autorités.
Ainsi :
Warning: L’usage de Db()->update() et ses soeurs sera globalement refusées en revue de code.
Important: Il peut y avoir des exceptions mais celles-ci devront être dûment justifiées avec un court commentaire dans le code pour que les prochains lecteurs sachent qu’il s’agit d’une exception acceptée après discussion.
Note: Pour rappel, lorsqu’on utilise SimpleUpdateData(), il faut éviter de passer les champs NULL en paramètres pour ne pas surcharger la table varsetmonitor pour rien.