Parole d’expert – Code smell : l’indentation excessive du code

Tournez votre tête de quatre-vingt dix degrés vers la droite. Si votre code ressemble à l’Everest, accompagné de ses petites soeurs Nuptse et Lhotse, alors votre code est probablement trop indenté, et trop long : c’est un code smell. Les « code smells » sont un indicateur facile à déceler, souvent symptômes d’un problème sous-jacent plus important.

Houston, nous avons un problème.

Dans cet article j’ai envie de vous parler du « code smell » très courant suivant: celui d’avoir trop de niveaux d’indentation dans le code.

Je vais utiliser ici du code java pour illustrer mes exemples, mais ce problème transcende les différences entre la plupart des langages de programmation. Nous allons ensuite retravailler le code d’exemple afin de résoudre ce problème d’indentation excessive du code, sans examiner d’autres axes d’amélioration du code.

Vous pouvez retrouver tous les exemples de code présentés ci-dessous sur github : https://github.com/Draluy/code_smell_indentation.

Dans mon expérience, une indentation excessive du code est due à un mélange de trois facteurs :

1. Le filtrage d’une liste de données en utilisant des conditions « if »
2. L’itération sur un ensemble de données trop grand, ce qui nécessite son filtrage par la suite
3. L’itération sur des données puis l’itération à nouveau sur un sous-emsemble de ces données

Voyons comment résoudre chaque cas indépendamment. Les solutions que je propose peuvent être  combinées entre elles afin de simplifier du code qui serait un assemblage des facteurs présentés plus haut.

Afin d’illustrer mes deux premiers facteurs je vais utiliser ce snippet de code, tiré d’un cas réel. Il n’est souvent pas nécessaire de comprendre le métier qui se cache derrière du code pour pouvoir le rendre plus lisible, et cet exemple n’est pas une exception : ne vous attachez pas à comprendre ce que fait un LogRequestor, cela n’est pas nécessaire.

for (int i = 0; i < logRequestors.length; i++) {
    if (logRequestors[i] != null && logRequestors[i].getClass().equals(alclass)) {
        // Make sure there's another stack frame.
        if (i + 1 < logRequestors.length) {
            LogRequestor logRequestor = logRequestors[i + 1];
            if (logRequestor.getClass().equals(alclass)) {
                logRequestors[i + 1] = null;
            } // Also AbstractLogger
        } // Found something that wasn't abstract logger
    } // check for abstract logger
}

L’auteur semble s’être rendu compte de la difficulté à comprendre ce code, puisque il ajoute en fin d’accolades des commentaires pour ne pas se perdre dedans.

1. Le filtrage des données à l’aide de ifs

Même si il est facile de coder de cette façon, des ifs imbriqués sont souvent le signe qu’une règle de filtrage est en train d’être appliquée sur les données en entrée, mais sans dire explicitement laquelle. Il faut se pencher sur le code pour la comprendre.

Dans l’exemple plus haut, on peut voir trois conditions imbriquées. La première vérifie que la valeur courante du tableau « ste » est d’un certain type, et la seconde que nous ne nous trouvons pas à la fin du tableau « ste ». Si ces deux conditions sont réunies alors on va regarder la valeur suivante dans le tableau, et la mettre à null si elle à la même classe que la valeur courante du tableau ste.

La solution dans ce cas consiste, à partir de ces simples observations, à encapsuler les conditions dans des méthodes nommées le plus explicitement possible. Cela permet de diminuer le nombre d’indentations en rendant le code plus lisible, jugez plutôt :

for (int i = 0; i < logRequestors.length; i++) {
    LogRequestor curentLogRequestor = logRequestors[i];
    if (isOfClass(curentLogRequestor, alclass) && !isLastValue(logRequestors, i)) {
        LogRequestor nextLogRequestor = logRequestors[i + 1];
        if (isOfClass(nextLogRequestor, alclass)) {
            logRequestors[i + 1] = null;
        }
    }
}

On peut aller plus loin et n’utiliser q’une seule condition :

for (int i = 0; i < logRequestors.length; i++) {
    LogRequestor curentLogRequestor = logRequestors[i];
    if (isOfClass(curentLogRequestor, alclass)
          && !isLastValue(logRequestors, i)
          && isOfClass(logRequestors[i + 1], alclass)) {
        logRequestors[i + 1] = null;
    }
}

Ou, si l’on préfère les streams java :

IntStream.range(0, logRequestors.length)
    .filter(i -> isOfClass(logRequestors[i], alclass))
    .filter(i -> !isLastValue(logRequestors, i))
    .filter(i -> isOfClass(logRequestors[i + 1], alclass))
    .forEach(i -> logRequestors[i + 1] = null);
return logRequestors;

La règle qui se cachait dans le code est maintenant écrite en toutes lettres, et l’on peut lire le code naturellement. Néanmoins, cela se fait dans ce cas au détriment d’une condition composée assez longue. Je laisse au lecteur le choix de la solution qui le convainc le plus. Dites-nous quel est votre avis!

2. L’itération sur trop de données

Le même exemple peut servir de démonstration à ce problème. Dans cet exemple le développeur n’est pas intéressé par tous les LogRequestor, mais seulement ceux appartenant à une classe précise et n’étant pas derniers dans le tableau « ste ».

Ceci peut être résolu par une méthode à deux étapes : filtrer d’abord les données qui sont pertinentes, puis itérer sur celles-ci en leur appliquant les règles métier si nécessaire. Ceci a comme avantage de non seulement réduire le niveau d’indentation, mais cela permet de fragmenter le code en des méthodes plus petites, plus faciles à lire et tester.

Le résultat de cette méthode est quelque chose comme ceci :

final LogRequestor[] filteredLogRequestors = ArrayUtils.getAllButLastElement (logRequestors);
List<String> logRequestorIndexes =  ArrayUtils.getIndexesOfElementsOfClass(filteredLogRequestors, alclass);

for (Integer index : logRequestorIndexes ){
    LogRequestor logRequestor = logRequestors[index];
    if (logRequestor != null) {
        logRequestors[index + 1] = null;
    }
}

les deux premières lignes prennent tous les objets du tableau sauf le dernier, avant de récupérer les index des objets qui appartiennent à la classe qui nous intéresse. Ensuite, on met à null les valeurs suivantes, dans une boucle maintenant bien plus simple.

Ou, si l’on préfère les streams java :

final LogRequestor[] filteredLogRequestors = ArrayUtils.getAllButLastElement (logRequestors);
List<Integer> logRequestorIndexes =  ArrayUtils.getIndexesOfElementsOfClass(filteredLogRequestors, alclass);

logRequestorIndexes.stream()
    .filter(i -> logRequestors[i] != null)
    .forEach(i -> logRequestors[i + 1] = null);

En filtrant les données a priori, on peut bien se rendre compte qu’en réalité on n’avait pas besoin d’itérer sur une liste d’objets, mais connaître leur position dans le tableau suffit. Réduire le périmètre des données utilisées va également permettre d’utiliser moins de mémoire plus rapidement, et donc d’être plus performant.
Enfin, la fragmentation du code en plusieurs méthodes le rend plus testable : il est plus facile de tester les trois méthodes résultantes séparément, car elles sont plus simples et plus courtes.

3. Itérer sur des données, puis à nouveau sur un sous-emsemble

Voici un exemple de code qui comporte deux boucles for imbriquées :

JSONObject json = new JSONObject(loadJSONFromAsset("recomendations.json"));

for (Iterator it = json.keys(); it.hasNext(); ) {
    String key = it.next();
    for (int i = 0; i < json.getJSONArray(key).length(); i++) {
        if (recommendations.get(key) == null)
            recommendations.put(key, new ArrayList());
            recommendations.get(key).add(json.getJSONArray(key).get(i).toString());
        }
    beerNames.add(key);
}

Du code qui utilise des boucles for imbriquées peut toujours être remplacé par une série d’étapes consécutives, même lorsque le code le plus imbriqué dépend de données appartenant aux boucles qui les encadrent.

Ici, nous avons un exemple de ce genre de code : le code le plus imbriqué dépend de la variable « json », la variable « key » lue dans la boucle extérieure, mais également de la variable « i » lue dans la boucle imbriquée.

Afin de simplifier le problème, remplaçons le contenu de la boucle imbriquée par une fonction « doStuff » pour le moment.

for (Iterator it = json.keys(); it.hasNext(); ) {
    String key = it.next();
    for (int i = 0; i < json.getJSONArray(key).length(); i++) {
        doStuff(json, key, i);
    }
    beerNames.add(key);
}

Nous pouvons maintenant plus facilement constater qu’il se passe deux choses: une itération sur les clés de l’objet json pour remplir la liste « beerNames », puis, avec chacun de ces éléments, la récupération d’un tableau et l’itération de celui-ci. Séparons donc ces deux étapes :

//Première étape : itérer sur les noms de bière
json.keys().forEachRemaining(beerNames::add);

//Deuxième étape, itérer sur les reommendations
beerNames.forEach(key -> {
    for (int i = 0; i < json.getJSONArray(key).length(); i++) {
        doStuff(json, key, i);
    }
});

Maintenant le code est un peu plus clair et testable, mais nous pouvons encore l’améliorer. En revenant au premier exemple de cette section, nous pouvons voir que la fonction doStuff fait une itération sur les recommendations.

Nous n’avons en réalité pas besoin du paramètre « i » : il est utilisé simplement pour récupérer la prochaine recommendation.

Une approche qui peut être utilisée tout le temps dans le cas où la boucle intérieure a besoin de données externes est de faire un tuple (ou un objet contenant les attributs nécessaires) contenant les valeurs nécessaires dans la boucle imbriquée, comme ceci :

//first step, populate the beer names and create the tuples
Map<String, JSONArray> recommendationsMap = Streams.stream(json.keys())
    .peek(beerNames::add)
    .collect(Collectors.toMap(Function.identity(), json::getJSONArray));

//second step, iterate over the recommendations
recommendationsMap.forEach(this::addRecommendations);

Ceci est une meilleure solution car nous avons une première étape qui prépare les données de la seconde étape, tout en remplissant la liste « beerNames ». Cela évite une nouvelle itération sur le json, et découple au plus tôt le code de ce format.

Conclusion

En utilisant ces astuces vous pourrez transformer presque n’importe quel code trop imbriqué en une série d’étapes clairement définies et nommées, les rendant faciles à lire et tester.