"Si ça marche, ce n'est pas une erreur"

Un des points qui me tiennent à coeur, c'est la qualité des développements. Et une grande partie de ma fonction consiste justement à veiller à cette qualité. La question toutefois est de savoir ce qu'est la qualité d'une application.

Un incident récent suggère que pour certains, il suffit que l'application fasse ce qu'on lui demande. Je suis d'accord que c'est un point essentiel, mais il ne suffit cependant pas à attribuer un label de qualité à une application.

La preuve avec un exemple bien réel.

L'analyse quotidienne faite par Sonar révèle un problème potentiel sur une application. Voici le code :

Integer i = Integer.valueOf(0);
Integer j = Integer.valueOf(1)
//Opérations sur i et j
if(i == j){
    //la suite du code

Le problème, c'est le "==" entre deux objets. Sonar soupçonne, avec raison, que c'est l'égalité des objets qui est testée et non l'égalité des références. Le code correct est donc:

if(i.equals(j))

C'est la correction la plus directe. Néanmoins, dans le cas présent, une meilleur approche est de transformer les Integer en int car ils ne se justifient pas et entraînent une perte de performance.

L'erreur est signifiée au développeur qui hausse les épaules et répond: "pourtant, ça marche".

Le fait est que la classe Integer, pour des valeurs comprises entre -128 et 127 utilise un cache, à condition de passer par valueOf (ce qui est le cas ici, mais aussi des opérations de boxing/unboxing).
Dans ces conditions, plusieurs Integer encapsulant le même "int" compris entre ces valeurs auront la même référence. Or, dans les opérations effectuées ci-dessus avec i et j, leurs valeurs sont comprises entre 0 et environ 10. Conséquence: bien qu'incorrect d'un point de vue OO, le test fonctionne.

Le développeur se tourne vers son chef d'équipe qui rétorque: "Si ça fonctionne, je ne vois pas où est le problème. De toute façon, on n'a pas le temps (sic)."

Au-delà de l'incident, on est sans doute tenté de se poser la question "pourquoi modifier ce code puisque ça marche?".

Voici quelques réponses:

1. parce que si les valeurs des Integer dépassent les limites du cache, le code ne fonctionnera plus
2. parce que si les valueOf sont remplacés par des new Integer(), ça ne fonctionnera plus
3. parce que c'est incorrect

Les objections à ces arguments sont d'intéressantes.

En ce qui concerne le premier point, le développeur fait remarquer qu'il est "peu probable" que les valeurs dépasseront 10 ou 12. De son côté, le chef d'équipe explique que, dans une approche "pragmatique", lorsque ça ne fonctionnera plus, la correction sera apportée.

Je ne peux être d'accord avec ces deux objections. "Peu probable" me semble "peu rassurant". Quant à l'approche qui consiste à réparer lorsque ça posera problème, elle risque d'entraîner une perte de temps pour retrouver où ça ne marche pas.

En ce qui concerne le deuxième point, c'est simple. Puisque nous (la cellule d'architecture) recommandons l'usage de valueOf plutôt que de new Integer(), la problème ne se présentera pas.

Quant à la troisième réponse, l'objection est simple: en quoi est-ce une erreur, puisque ça marche?

Le plus terrible dans cette histoire, c'est que la correction aurait pris, commit sur Subversion compris, une trentaine de secondes. Notre discussion a duré un quart d'heure.

Alors, c'est quoi finalement la qualité? C'est un sujet sur lequel j'aurai l'occasion de revenir.