Accueil > Non classé > Refactoring par la pratique

Refactoring par la pratique

Dans cet article, nous allons procéder à un refactoring sur un exemple concret, afin de mettre en exergue quelques bonnes pratiques.

Défi : comprendre du code inélégant*

*Edit : j’avais initialement écrit “imb*table”, mais il paraît que ça ne fait pas très sérieux :-P

Êtes-vous capable de comprendre ce que fait la méthode computeConvexHull() en moins de 3 minutes ?


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
public List<GeoPoint> computeConvexHull(Set<GeoPoint> points) {

    // Path list
    ArrayList<GeoPoint> path = new ArrayList<GeoPoint>();

    // Pick the min longitude point as first
    GeoPoint first = null;
    for (GeoPoint point : points) {
        if (first == null || point.getLongitude() < first.getLongitude()) {
            first = point;
        }
    }

    if (first != null) {
        path.add(first);
    } else {
        throw new IllegalArgumentException("Can't retrieve the western point");
    }

    boolean right = true;
    boolean loop = true;

    while (loop) {

        GeoPoint current = path.get(path.size() - 1);

        GeoPoint next = null;
        double nextLatitudeDiff = 0;
        double nextLongitudeDiff = 0;
        boolean recordNext = false;
        for (GeoPoint point : points) {
            if (!point.equals(current)) {
                double latitudeDiff = point.getLatitude() - current.getLatitude();
                double longitudeDiff = point.getLongitude() - current.getLongitude();
                if (longitudeDiff == 0 || longitudeDiff > 0 == right) {
                    if (next == null) {
                        recordNext = true;
                    } else {
                        // Compare the 'a' in the linear equation Y = a.X
                        // that correspond to the line slope.
                        boolean negativeSlope = latitudeDiff * nextLongitudeDiff - nextLatitudeDiff * longitudeDiff < 0;

                        if (negativeSlope) {
                            recordNext = true;
                        }
                    }

                    if (recordNext) {
                        recordNext = false;
                        next = point;
                        nextLatitudeDiff = latitudeDiff;
                        nextLongitudeDiff = longitudeDiff;
                    }
                }
            }
        }

        if (next != null) {

            // Check abnormal case
            if (path.size() > points.size() + 1) {
                throw new RuntimeException("Convex hull computation failed");
            }

            path.add(next);

            /*
             * If we are not back to the first point, let's continue finding
             * the path.
             *
             * Otherwise, do nothing => going back from the stack, this is
             * actually the end point of the recursive algorithm.
             */


            if (path.get(0) == next) {
                loop = false;
            }

        } else {
            // We have reached the point on the right, let's go back!
            right = !right;
        }
    }

    return path;
}

Alors ? Checkstyle lui donne une complexité cyclomatique de 16. Plutôt difficile d’y voir clair. Moi en tout cas, je n’en suis pas capable.
Et pourtant, j’en suis l’auteur :-). Critique de code bien ordonnée commence par soi-même !

Le développeur, lecteur à temps plein

On entend souvent dire que produire un code de qualité, c’est un coût à court terme et un gain à long terme. C’est faux.

En tant que développeur, je passe l’essentiel de mon temps à lire du code, et proportionnellement peu à en écrire. En effet, avant chaque modification, je parcours le code pour en comprendre les tenants et les aboutissants, et décider des modifications que je vais réaliser.

Étudiez la manière dont vous développez, vous verrez c’est édifiant ;-) .

Puisqu’un développeur passe l’essentiel de son temps à lire du code, le meilleur moyen de gagner en productivité, c’est avoir un code qui soit facile à comprendre.

Améliorer la qualité du code, c’est un gain immédiat, un point c’est tout.

Pas de refactoring sans tests

Il est impossible de produire un code de qualité dès le départ. Ne vous morfondez pas, ce n’est pas grave.

Pour améliorer le code, rien de tel que le refactoring, qui consiste à transformer le code sans en modifier le fonctionnement extérieur. Des tests unitaires sont alors nécessaires, car ils permettent d’exprimer le comportement du code et de garantir qu’il n’a pas varié. Les tests doivent être écrits avant tout refactoring.

En fait, idéalement, vous faites du TDD et votre code est déjà testé. Il n’est cependant pas rare de rejoindre une équipe où personne n’a “eu le temps” d’en écrire un seul.

Mais revenons à notre code incompréhensible…

Enveloppe convexe, mékeskidit ?

Comme son nom l’indique :-P, la méthode computeConvexHull() permet de calculer l’enveloppe convexe (l’emballage) d’un ensemble de points géolocalisés.

Enveloppe convexe

Le principe de l’algorithme est de partir du point le plus à l’ouest, trouver le chemin de l’enveloppe dans le sens anti-horaire jusqu’au point le plus à l’est, puis faire de même dans l’autre sens.

Toutefois, nous ne connaissons toujours pas précisément son fonctionnement.

Des tests pour comprendre et exprimer ce que fait le code

Afin d’y parvenir, je vous propose d’écrire des tests. C’est un bon moyen pour comprendre et documenter le fonctionnement du code, et contrairement aux commentaires, les tests évoluent au même rythme que le code.

Pour ne pas alourdir l’article, je vous laisse consulter les tests en question.

Quand cesser d’écrire des tests ? Quand vous êtes certains d’avoir couvert tous les comportements possibles du code. Vous pouvez par exemple utiliser EclEmma pour obtenir le pourcentage de couverture des classes que vous testez. 100% est un minimum, cela ne garantit pas que vous n’avez rien oublié.

Écrire des tests, c’est aussi découvrir des bugs

Ceux d’entre vous qui ont lu les tests se sont certainement exclamé : “Hey, c’est de la triche, il y a des tests en commentaire !”.

Il s’avère qu’en tentant d’exprimer le comportement que j’attendais de ce code à travers mes tests unitaires, j’ai rencontré des bugs.

J’ai donc écris du code mettant en évidence ces comportements inattendus :


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
@Test(expected = TimeoutException.class)
public void onePointSetLoopEndlessly() throws Exception {
    final Set<GeoPoint> points = new HashSet<GeoPoint>();
    points.add(new GeoPoint(10, 10));

    timeoutAfterDelay(300, new Runnable() {
        public void run() {
            hullHelper.computeConvexHull(points);
        }
    });
    Assert.fail("One point set should loop endlessly (timeout from tests after 300 ms)");
}
private void timeoutAfterDelay(int delayInMs, Runnable codeToRun) throws TimeoutException, InterruptedException,
        ExecutionException {
    ExecutorService executor = Executors.newCachedThreadPool();
    FutureTask<?> future = new FutureTask<Object>(codeToRun, null);
    executor.submit(future);
    future.get(delayInMs, TimeUnit.MILLISECONDS);
}

@Test(expected = RuntimeException.class)
public void computationFail() {
    Set<GeoPoint> points = new HashSet<GeoPoint>();

    GeoPoint northWest = new GeoPoint(10, 5);
    GeoPoint southWest = new GeoPoint(0, 0);
    GeoPoint southEast = new GeoPoint(5, 10);
    GeoPoint northEast = new GeoPoint(15, 10);

    points.add(northWest);
    points.add(southWest);
    points.add(southEast);
    points.add(northEast);

    hullHelper.computeConvexHull(points);
    Assert.fail("Should fail when there are two eastern points with same longitude");
}

@Test(expected = NullPointerException.class)
public void pointsNullThrowsNullPointerException() {
    hullHelper.computeConvexHull(null);
    Assert.fail("Null set should throw a NullPointerException");
}

Avant de refactorer, il faut corriger les bugs :


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
// Tout d'abord, en vérifiant les paramètres d'entrée.
public List<GeoPoint> computeConvexHull(Set<GeoPoint> points) {
    if (points == null) {
        throw new IllegalArgumentException();
    }
    if (points.size() == 1) {
        return new ArrayList<GeoPoint>(points);
    }
// [...]

// Par ailleurs, il s'avère que ce if n'était pas assez complexe...
if (longitudeDiff == 0 || longitudeDiff > 0 == right)

// Il devient :
if (longitudeDiff == 0 && latitudeDiff < 0 == right || longitudeDiff > 0 == right)
// Beaucoup plus clair, isn't it ?

Je vous laisse parcourir la version corrigée. De même pour les tests unitaires, qui désormais passent tous.

Refactoring : principe

Un refactoring en bonne et due forme, c’est une suite de petites modifications avec exécution des tests entre chaque modification, pour s’assurer que l’on ne change pas le comportement.

Pour réaliser ce refactoring, j’ai mis en œuvre les principes suivants :

  • Choisir des noms descriptifs pour les classes, méthodes et variables.
  • Minimiser le nombre d’arguments des méthodes, maximum 3 arguments. Si une méthode utilise les attributs d’un objet passé en paramètre, elle doit devenir une méthode d’instance sur cet objet. Lorsque des méthodes d’une même classe se passent le même argument les unes après les autres, il faut transformer cet argument en variable d’instance.
  • Pas d’arguments de sortie : les méthodes ne modifient pas l’état des arguments qui leurs sont fournis.
  • Pas de redondance : s’il y a de la redondance, il y a probablement une opportunité d’abstraction manquée.
  • Séparation verticale : les variables et les fonctions sont définies au plus près de leur utilisation. Les méthodes utilisées par une méthode du niveau d’abstraction supérieur doivent être placées immédiatement en dessous de celle-ci.
  • Variables explicatives : passer par des variables intermédiaires avec des noms explicites, plutôt qu’écrire des méthodes d’une seule ligne.
  • Limiter le nombre de switch.
  • Chaque méthode fait une seule chose, il est préférable d’écrire de nombreuses méthodes de quelques lignes avec des noms significatifs.

Voici comment j’ai procédé :

  • J’ai tout d’abord décomposé le code en plusieurs méthodes isolant les différentes étapes de l’algorithme.
  • Puis j’ai supprimé les arguments de sortie.
  • J’ai ensuite créé des classes encapsulant les différents traitements.
  • Et ainsi de suite :  j’ai transformé le code par itérations successives, en appliquant chacun des principes listés précédemment. Il ne faut pas tout faire d’un coup, mieux vaut réaliser une petite modification, exécuter les tests, commiter, et continuer.
  • Au fur et à mesure des transformations, je n’ai pas hésité à créer de nouvelles classes. Cela a temporairement augmenté la base de code, mais il s’avère qu’une fois les responsabilités bien séparées, de nombreuses simplifications apparaissent.

Pour finir, j’ai réalisé une dernière transformation qui ne plaira pas forcément à tout le monde : créer des classes d’algorithme stateful, qui travaillent sur des variables d’instance plutôt que sur des paramètres.

Cela permet de limiter grandement le nombre de paramètre, et le code devient alors une succession d’appels de méthode. Il faut donc soigneusement choisir les noms de méthodes, et limiter au maximum la taille des classes afin de ne pas perdre le fil. De plus, par nature ces classes ne sont pas threadsafe.

Le refactoring n’est donc pas un “ravalement de façade”, mais bien une succession de transformations qu’on ne peut prévoir d’avance. Je n’avais pas la moindre idée du résultat auquel je souhaitai arriver quand j’ai commencé à réaliser ces modifications.

Enfin, il est important de se limiter dans le temps, car il est difficile de définir des objectifs mesurables pour un refactoring.

Refactoring : résultat

La méthode computeConvexHull() est désormais bien allégée :


1
2
3
4
5
6
7
8
9
10
11
public List<GeoPoint> computeConvexHull(Set<GeoPoint> points) {
    GeoPointSet geoPointSet = new GeoPointSet(points);

    geoPointSet.checkPreconditions();
    if (geoPointSet.hasOnePoint()) {
        return geoPointSet.copyAsList();
    } else {
        HullFinder hullFinder = new HullFinder(geoPointSet);
        return hullFinder.findAntiClockwiseHull();
    }
}

L’algorithme est compréhensible à travers quelques méthodes clés. Tout d’abord, dans la classe HullFinder :


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
public List<GeoPoint> findAntiClockwiseHull() {
    findBounds();
    findEastPath();
    findWestPath();
    return assembleHull();
}

private void findBounds() {
    Bounds bounds = geoPointSet.findBounds();
    westernPoint = bounds.getWesternPoint();
    easternPoint = bounds.getEasternPoint();
}

private void findEastPath() {
    eastPath = eastPathFinder.findPath(westernPoint, easternPoint);
}

private void findWestPath() {
    westPath = westPathFinder.findPath(easternPoint, westernPoint);
}

private List<GeoPoint> assembleHull() {
    List<GeoPoint> hull = new ArrayList<GeoPoint>;
    hull.addAll(eastPath);
    hull.addAll(westPath);
    hull.add(westernPoint);
    return hull;
}

Et pour finir, dans la classe PathFinder :


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
public List<GeoPoint> findPath(GeoPoint startPoint, GeoPoint endPoint) {
    initPath(startPoint);
    initLastFound(startPoint);
    findNextPoint();

    while (!endReached(endPoint)) {
        addBestCandidateToPath();
        findNextPoint();
    }

    return path;
}

private void findNextPoint() {
    PositionDelta bestDelta = null;
    GeoPoint bestPoint = null;
    for (GeoPoint point : geoPointSet) {
        PositionDelta delta = buildDelta(lastFoundPoint, point);
        if (delta.isBetterCandidate(bestDelta)) {
            bestPoint = point;
            bestDelta = delta;
        }
    }
    lastFoundPoint = bestPoint;
}

private void addBestCandidateToPath() {
    path.add(lastFoundPoint);
}

Pour consulter l’intégralité du code, il faut aller lire les sources;).

Remarquez l’absence de commentaires. Les noms de méthodes et de variables permettent d’exprimer ce que fait le code.

Le plus souvent, les commentaires au sein du code sont une erreur. Ils n’évoluent jamais à la même vitesse que le code.

Quand vous lisez un code complexe, plutôt que d’y insérer des commentaires qui expriment ce qu’il fait (et donc dupliquer l’information), il est préférable de le modifier pour rendre son fonctionnement explicite.

Par ailleurs, cet algorithme n’est certainement pas la meilleure manière de calculer une enveloppe convexe. J’ai souhaité en conserver le principe général plutôt que de changer d’implémentation, pour que l’exemple garde tout son sens. Si vous avez mieux, lâchez vos coms ;-) !

Conclusion

À travers cette mise en pratique, j’espère vous avoir donné un aperçu des différents mécanismes à mettre en œuvre au cours d’un refactoring.

C’est loin d’être facile, et cela demande du courage. Il est bien plus aisé de laisser le code se délabrer.

Quand un collaborateur vous balance “touche pas, ça marche“, le code est probablement pourri. Mais ce n’est pas parce que c’est un projet poubelle qu’il ne faut rien faire, au contraire ! Pensez juste à prendre des radiographies du patient ;-) .

Je me suis largement inspiré de Clean Code pour écrire cet article. Ce livre a remis en question mes habitudes de développement, j’ai pris une claque. À mettre entre toutes les mains !

Share
  1. 03/08/2010 à 15:58 | #1

    Ah, merci tout particulièrement pour un point précis, le commentaire sur les commentaires ;)
    Je travaille dans un environnement où le checkstyle est une règle sacro-sainte, avec une paramétrie imposée et des warnings checkstyle pour le moindre commentaire javadoc manquant, du coup je me retrouve à longueur de journées avec du code contenant des kilomètres de javadoc plus ou moins automatiquement générée (CTRL+ALT+J), souvent pas à jour quand même genre sur le dernier paramètre ajouté (donc warning checkstyle quand même), très souvent inutile (une javadoc sur un getter/setter standard ou sur une méthode non-exposée totalement explicite par son nom et ses paramètres, quel intérêt ? Mais je prêche dans le désert …).

    Sinon plutôt d’accord avec le reste de l’article. Je fais souvent aussi beaucoup de “petites méthodes” aux termes explicites, pour avoir un point d’entrée où à un moment on peut facilement voir tout le “sommaire” du traitement, et souvent beaucoup de classes. Mais à l’inverse, faire trop de petites classes, au bout d’un moment, çà empêche parfois les gens qui reprennent derrière d’avoir une vue d’ensemble …

    La répartition en packages a son importance aussi (j’ai déjà repris du code avec “1 seul package pour tout”, c’est une horreur … mais il faut un découpage “cohérent” sans avoir trop de packages différents pour autant.

    Tous mes refactoring ont systématiquement commencé par une remise à niveau du “canevas” de tests, pour avoir quelque chose de vraiment intégré (genre démarrer une base “embedded” en automatique au début des tests ou le thread qui démarre un simulateur de réponse sans que çà doive être fait à la main avant de lancer les tests, etc.).

    Pour les classes d’algorithme stateful, je suis partagé (en pratique je le fais rarement), je trouve que sur les objets complexes, on s’y perd assez vite, et on ne sait plus au niveau d’une méthode ce qui doit être manipulé ou même ce qui est accessible … mais c’est peut être le signe d’un mauvais découpage, l’objet devrait être rendu + simple, avec moins d’attributs.

    Je note le “Clean Code”, je ne connaissais pas (j’avais lu le “refactoring des applications J2EE” chez Eyrolles mais je n’en avais pas vraiment retiré grand chose).

  2. 03/08/2010 à 16:14 | #2

    @SRG Merci pour ton retour, c’est bien de pouvoir confronter les expériences. Effectivement, j’ai aussi droit à l’environnement Checkstyle “corporate”, avec par exemple commentaire obligatoire sur tous les noms de classes.

    Mettez en place des normes contraignantes, vos devs les contourneront…

    Du coup, c’est bien souvent :

    1
    2
    3
    4
    /**
     * {@link MaClasse}
     */

    public class MaClasse{

    Notez le @link, pour que le commentaire reste à jour ;-)

    Je reste aussi partagé pour les classes d’algo stateful, mais cela peut se révéler très utile quand on souhaite refactorer une méthode dont les variables sont entremêlées, et qu’il est impossible d’utiliser “extract method”.

  3. Rémi
    09/08/2010 à 13:38 | #3

    Sympa l’article!
    C’est toujours utile d’avoir un concentré des good practices à un endroit, je vais faire lire ça à notre stagiaire :)
    A plus PY!

  1. 03/08/2010 à 15:06 | #1
  2. 03/08/2010 à 15:43 | #2
  3. 17/08/2015 à 01:00 | #3
  4. 24/02/2017 à 23:24 | #4