Mes bonnes pratiques – Général
1. Les commentaires – malédiction et bénédiction
Les bons commentaires sont un art. Bien pensés, un atout ; mais bien souvent, rien du tout.
Un code auto-documenté ne signifie pas « aucun commentaire ». Le code doit exprimer lui-même le comment autant que possible ; les commentaires ajoutent le pourquoi, les contraintes qui ne sautent pas aux yeux et le contrat d’une API.
Voici quelques exemples de code auto-documenté, ainsi que de bons et de mauvais commentaires.
Un argument fréquent veut que les commentaires deviennent vite obsolètes. En réalité, le problème vaut tout autant pour les noms de variables et de méthodes !
Aïe – à ne jamais faire :
int age = 32; // set the value of the age integer to 32Auto-documenté : des noms de variables parlants, c’est la moitié du travail
Mauvais :
var dateTime = DateTimeUtil.getNow();Bon :
var now = DateTimeUtil.getNow();Un autre exemple :
Mauvais :
User user = this.getBruce();Mauvais :
// testing user without permission
User user = this.getBruce();Bon :
User userWithoutPermission = this.getBruce();Auto-documenté : des noms de méthodes parlants
Mauvais :
public String sanitizeInput(String input) {
// replace underscore with spaces
return input.replaceAll("_", " ");
}
Bon :
public String replaceUnderscoreWithSpaces(String input) {
return input.replaceAll("_", " ");
}
Auto-documenté : les méthodes privées
Mauvais : la méthode oblige le lecteur à déchiffrer en même temps la validation, le calcul et la règle métier.
public Invoice createInvoice(Order order, Customer customer) {
if (order.items().isEmpty()) {
throw new IllegalArgumentException("Order needs at least one item");
}
BigDecimal subtotal = order.items().stream()
.map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
.reduce(BigDecimal.ZERO, BigDecimal::add);
BigDecimal discount = customer.isPremium()
? subtotal.multiply(new BigDecimal("0.10"))
: BigDecimal.ZERO;
return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}Bon : la méthode publique raconte le déroulement. Des méthodes privées et stateless nomment chaque intention et rendent leurs dépendances visibles par leurs paramètres et leurs valeurs de retour.
public Invoice createInvoice(Order order, Customer customer) {
requireItems(order);
BigDecimal subtotal = calculateSubtotal(order.items());
BigDecimal discount = loyaltyDiscountFor(customer, subtotal);
return invoiceFor(order, subtotal, discount);
}
private static void requireItems(Order order) {
if (order.items().isEmpty()) {
throw new IllegalArgumentException("Order needs at least one item");
}
}
private static BigDecimal calculateSubtotal(List<OrderItem> items) {
return items.stream()
.map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
.reduce(BigDecimal.ZERO, BigDecimal::add);
}
private static BigDecimal loyaltyDiscountFor(Customer customer, BigDecimal subtotal) {
return customer.isPremium()
? subtotal.multiply(new BigDecimal("0.10"))
: BigDecimal.ZERO;
}
private static Invoice invoiceFor(
Order order,
BigDecimal subtotal,
BigDecimal discount
) {
return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}Chaque ligne n’a pas besoin de sa propre méthode. On extrait une idée cohérente qui peut être nommée avec précision. Des noms comme processData() ou handleStuff(), en revanche, n’apportent rien.
Règle d’or : commentez pourquoi quelque chose se produit, plutôt que ce qui se produit
On voit clairement ce qui se passe ici :
public Duration retryDelay(int retryCount) {
long seconds = Math.min(1L << retryCount, 60);
return Duration.ofSeconds(seconds);
}Mais pourquoi ?
Mieux :
public Duration retryDelay(int retryCount) {
// Cap the delay so an outage never blocks queued jobs for more than one minute.
long seconds = Math.min(1L << retryCount, 60);
return Duration.ofSeconds(seconds);
}Un autre exemple tiré de la vraie vie. On voit ici des méthodes qui affichent et masquent l’indicateur de chargement Ajax. Mais il y a quatre méthodes, et la raison n’est pas évidente au premier coup d’œil :
function showAjaxLoader() {
$('.layout-ajax-loader').children().first().show();
}
function hideAjaxLoader() {
$('.layout-ajax-loader').children().first().hide();
}
function suppressAjaxLoader() {
getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
}
function releaseAjaxLoader() {
getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
}Les méthodes semblent toutes très proches et manipulent visiblement la visibilité via CSS. Avec un court commentaire, les cas d’usage deviennent immédiatement clairs :
showAjaxLoader: function() {
$('.layout-ajax-loader').children().first().show();
},
hideAjaxLoader: function() {
$('.layout-ajax-loader').children().first().hide();
},
// take the control from primefaces and don't allow the spinner to show, no matter what
suppressAjaxLoader: function() {
getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
},
// reset the state after hiding, to give the control back to primefaces
releaseAjaxLoader: function() {
getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
},Un autre exemple :
Mauvais :
/**
* Sets the tool tip text.
*
* @param text the text of the tool tip
*/
public void setToolTipText(String text) {}Bon :
/**
* Registers the text to display in a tool tip. The text
* displays when the cursor lingers over the component.
*
* @param text the string to display. If the text is null,
* the tool tip is turned off for this component.
*/
public void setToolTipText(String text) {}Discussion lors de l’atelier du 21 juin 2021 : tous les développeurs s’accordent à dire qu’il vaut mieux éviter la Javadoc en général (cauchemar des paramètres, constamment obsolète et pénible à maintenir) et ne l’utiliser que lorsqu’un paramètre a vraiment besoin d’une explication, ou lorsqu’il faudrait sinon lire toute une longue méthode.
Exception à la règle d’or : commentez ce qui se produit – ici, c’est recommandé
Quand cela améliore nettement la lisibilité, parce que le code ne serait sinon pas clair :
let isAlwaysAllowedKey = (
e.which === 8 || /* BACKSPACE */
e.which === 35 || /* END */
e.which === 36 || /* HOME */
e.which === 37 || /* LEFT */
e.which === 38 || /* UP */
e.which === 39 || /* RIGHT*/
e.which === 40 || /* DOWN */
e.which === 46 || /* DEL*/
e.ctrlKey === true && e.which === 65 || /* CTRL + A */
e.ctrlKey === true && e.which === 88 || /* CTRL + X */
e.ctrlKey === true && e.which === 67 || /* CTRL + C */
e.ctrlKey === true && e.which === 90 /* CTRL + Z */
)Commentaires – les gros mots dans le code du noyau Linux
Pour finir, une petite anecdote amusante sur le noyau Linux – le nombre de jurons dans son code source :
https://www.vidarholen.net/contents/wordcount/#fuck*,shit*,damn*,idiot*,retard*
Amusez-vous bien à écrire des commentaires !
2. Les méthodes privées dans les longues routines sont dangereuses
Attention ! Dans le monde orienté objet d’aujourd’hui, les méthodes privées sont légion. En général, ce n’est pas un problème. Cela le devient lorsque ces méthodes accèdent à des champs, autrement dit à l’état, et qu’elles se trouvent dans de longues routines. De telles sous-méthodes devraient être privées et stateless. Sinon, elles ont l’inconvénient de souvent causer des ennuis lorsqu’on les appelle isolément. Tôt ou tard, un développeur aura l’idée d’appeler directement la sous-méthode. Les états illégaux sont l’une des principales causes de bugs. À ce sujet, un article intéressant sur quelques propos de John Carmack (id Software, Doom), qui explique que dans les applications où le code doit être extrêmement stable (les vols spatiaux, par exemple), les sous-routines sont souvent interdites, ce qui rend le code plus déterministe : http://number-none.com/blow/john_carmack_on_inlined_code.html
Cette anecdote ne s’applique que partiellement au monde moderne et stateless de Java/Spring, mais elle reste une bonne matière à réflexion.