Clean Code, Refactoring et Resharper (2)

Dans ce second article, nous allons aller un peu plus loin et commencer à réorganiser notre code. Nous allons nous attaquer à un kata dans son intégralité : la calcul du score de tennis. Le point de départ est le suivant :

using System;
using NUnit.Framework;

namespace Tennis
{
  class TennisGame1 : TennisGame
  {
    private int m_score1 = 0;
    private int m_score2 = 0;
    private string player1Name;
    private string player2Name;

    public TennisGame1 (string player1Name, string player2Name)
    {
      this.player1Name = player1Name;
      this.player2Name = player2Name;
    }

    public void WonPoint (string playerName)
    {
      if (playerName == "player1")
        m_score1 += 1;
      else
        m_score2 += 1;
    }

    public string GetScore ()
    {
      String score = "";
      int tempScore=0;
      if (m_score1==m_score2)
      {
        switch (m_score1)
        {
        case 0:
          score = "Love-All";
          break;
        case 1:
          score = "Fifteen-All";
          break;
        case 2:
          score = "Thirty-All";
          break;
        default:
          score = "Deuce";
          break;

        }
      }
      else if (m_score1>=4 || m_score2>=4)
      {
        int minusResult = m_score1-m_score2;
        if (minusResult==1) score ="Advantage player1";
        else if (minusResult ==-1) score ="Advantage player2";
        else if (minusResult>=2) score = "Win for player1";
        else score ="Win for player2";
      }
      else
      {
        for (int i=1; i<3; i++)
        {
          if (i==1) tempScore = m_score1;
          else { score+="-"; tempScore = m_score2;}
          switch(tempScore)
          {
          case 0:
            score+="Love";
            break;
          case 1:
            score+="Fifteen";
            break;
          case 2:
            score+="Thirty";
            break;
          case 3:
            score+="Forty";
            break;
          }
        }
      }
      return score;
    }
  }

}

Que voyons-nous ? Un constructeur, 2 méthodes publiques : WonPoint() et GetScore(), et quelques champs privés. En parcourant notre code de haut en bas, nous notons plusieurs choses grâce à Resharper :

• un using inutile
• des champs mal nommés et inutilisés (ce qui est louche, on y reviendra)
• beaucoup de chaines de caractères en dur
• la méthode WinPoint est courte et demandera pas ou peu de travail
• la méthode GetScore contient presque toute la logique de la classe, et qui demandera naturellement le plus d’attention.

On commencera par supprimer le using inutile et renommer les champs en les préfixant avec des underscores grâce au raccourci clavier Alt+Entrée. Les champs _mScore1 et _mScore2 sont initialisés explicitement à 0, ce qui est inutile, car c’est la valeur par défaut d’un entier. A vous de voir si vous préférez une initialisation explicite. Personnellement, je supprime.

Passons à la méthode WonPoint(). Nous allons remplacer la chaîne “player1” en dur par l’appel au champ _player1Name. Enfin, nous allons remplacer les += 1 par des ++, puisque c’est à ça que sert cet opérateur. Enfin, comme les opérations sont simplissimes, on peut inliner les if-else. Le résultat est le suivant :

public void WonPoint(string playerName)
{
    if (playerName == _player1Name) _mScore1++;
    else _mScore2++;
}

Attaquons-nous maintenant au gros oeuvre : la méthode GetScore(). Elle fait 50 lignes, c’est trop. De plus, nous voyons cohabiter des variables tempScore, score en plus des champs _mScore1 et _mScore2. Pas très clair. D’ailleurs, à quoi sert tempScore ? Il faut descendre bas dans le code pour voir son utilisation. De plus, elle n’est utilisée que dans une partie localisée de la méthode. Nous allons donc rapprocher sa déclaration de son utilisation. Ici encore, le raccourci Alt+Entrée de Resharper nous propose l’option suivante : Move “declaration nearer to usage”. Si l’indentation est mauvaise, le raccourci Ctrl K+D permet de remettre ça au propre.

Nous avons maintenant une situation classique : notre méthode est composée de 3 parties isolées dans des blocs if-else. On gère en premier les cas d’égalité de score, puis les fins de jeu, et enfin le cas standard. Nous allons donc découper notre grosse méthode en plusieurs petites pour améliorer la lisibilité et la maintenabilité.

Nouveau type de refactoring : extractions de méthodes (Ctrl R+M ou Ctrl-Alt-R + flèche bas)

Nous avons donc extrait une méthode GetScoreForEquality qui gère, comme son nom l’indique, les cas d’égalité. Remarquons au passage que le dialogue expose plusieurs options permettant de générer le code voulu. Nous avons, par exemple, supprimé le paramètre score entrant, puisque nous n’en avons pas besoin. Nous allons de la même façon extraire des méthodes GetScoreForEndOfGame.

La dernière méthode est plus complexe à gérer et demande à être étudiée avant de l’extraire. Tout d’abord, nous voyons une boucle. Étrange. Que vient faire une boucle ici ? Puis nous regardons les indices de début et fin de boucle. Elle commence à 1 et s’arrête avant d’arriver à 3. Deux tours de boucle, donc. Hmm. Puis nous voyons que tout ce qu’elle fait, c’est de traduire les chiffres des scores des deux joueurs en lettres et en les séparant par un tiret… Très compliqué pour pas grand chose. On va améliorer ça. Débarrassons-nous d’abord de ce vilain switch en introduisant un bête tableau qui nous servira à la conversion.

var possibleScores = new[] {"Love", "Fifteen", "Thirty", "Forty"};
score = string.Format("{0}-{1}", possibleScores[_mScore1], possibleScores[_mScore2]);

Afin de finir, nous allons adopter pour des early returns, plutôt que de l’affectation à une variable score retournée uniquement à la fin. C’est en effet plus simple pour nous pauvres humains d’avoir à mémoriser uniquement les chemins qui nous intéressent plutôt que tous les chemins possibles, au risque de s’y perdre.

Notre méthode GetScore est maintenant bien plus simple :

public string GetScore()
{
    if (_mScore1 == _mScore2)
        return GetScoreForEquality();

    if (_mScore1 >= 4 || _mScore2 >= 4)
        return GetScoreForEndOfGame();

    return GetScoreStandard();
}

Cependant, les 3 méthodes extraites sont encore perfectibles. Commençons (logiquement) par la dernière :

private string GetScoreStandard()
{
    var possibleScores = new[] {"Love", "Fifteen", "Thirty", "Forty"};
    string score = string.Format("{0}-{1}", possibleScores[_mScore1], possibleScores[_mScore2]);
    return score;
}

Nous pouvons voir que la conversion de chiffres en lettres des scores est également présente dans GetScoreForEquality(). Le tableau de conversion est donc commun aux deux. Nous allons le transformer en champ readonly.

Nous en avons profité pour inliner la variable, de façon à la retourner directement. Ce type de refactoring peut varier selon les goûts. En effet, avoir une variable locale à une méthode permet de poser facilement des espions et peut se révéler pratique pour des méthodes complexes. Mais depuis VS2013, il est possible d’espionner la valeur de retour d’une méthode. C’est plutôt ce mode de fonctionnement que je privilégie aujourd’hui, de façon à alléger au maximum le code. Une autre question se pose maintenant : puisque notre méthode ne fait qu’une ligne et n’est appelée que d’un seul endroit, a-t-elle un sens ? Je penche pour oui, car ça permet de garder une méthode GetScore cohérente et facile à lire.

La méthode GetScoreForEndOfGame() est plutôt bien telle quelle. Je remplace seulement les “player1” et “player2” en dur par des références aux champs _player1Name et _player2Name. J’en profite également pour rendre ces deux champs readonly, car ils ne sont modifiés que dans le constructeur.

Dernière étape : la méthode GetScoreForEquality(). Nous allons chercher à nous débarrasser là encore du switch (toujours regarder suspicieusement un switch). Notre tableau de conversion _possibleScores va nous être utile, car il va gérer les cas 0, 1 et 2.

private string GetScoreForEquality()
{
    string score;
    switch (_mScore1)
    {
        case 0:
            score = _possibleScores[0] + "-All";
            break;
        case 1:
            score = _possibleScores[1] + "-All";
            break;
        case 2:
            score = _possibleScores[2] + "-All";
            break;
        default:
            score = "Deuce";
            break;
    }
    return score;
}

On voit un pattern se dessiner. Continuons, et remplaçons les index 0, 1 et 2 par l’appel à la variable _mscore1 :

private string GetScoreForEquality()
{
    string score;
    switch (_mScore1)
    {
        case 0:
            score = _possibleScores[_mScore1] + "-All";
            break;
        case 1:
            score = _possibleScores[_mScore1] + "-All";
            break;
        case 2:
            score = _possibleScores[_mScore1] + "-All";
            break;
        default:
            score = "Deuce";
            break;
    }
    return score;
}

On a maintenant 3 lignes identiques, ce qui permet d’écrire ceci :

private string GetScoreForEquality()
{
    if (_mScore1 >= 2) return _possibleScores[_mScore1] + "-All";
    return "Deuce";
}

En me relisant, je me rends compte que les champs _mScore1 et _mScore2 sont mal nommés et incohérents avec _player1Name et _player2Name. Je les renomme en _player1Score et _player2Score. Le code final est donc le suivant :

namespace Tennis
{
    class TennisGame1 : TennisGame
    {
        private int _player1Score;
        private int _player2Score;
        private readonly string _player1Name;
        private readonly string _player2Name;
        private readonly string[] _possibleScores = { "Love", "Fifteen", "Thirty", "Forty" };

        public TennisGame1(string player1Name, string player2Name)
        {
            _player1Name = player1Name;
            _player2Name = player2Name;
        }

        public void WonPoint(string playerName)
        {
            if (playerName == _player1Name) _player1Score++;
            else _player2Score++;
        }

        public string GetScore()
        {
            if (_player1Score == _player2Score)
                return GetScoreForEquality();

            if (_player1Score >= 4 || _player2Score >= 4)
                return GetScoreForEndOfGame();

            return GetScoreStandard();
        }

        private string GetScoreStandard()
        {
            return string.Format("{0}-{1}", _possibleScores[_player1Score], _possibleScores[_player2Score]);
        }

        private string GetScoreForEndOfGame()
        {
            string score;
            int minusResult = _player1Score - _player2Score;
            if (minusResult == 1) score = "Advantage " + _player1Name;
            else if (minusResult == -1) score = "Advantage " + _player2Name;
            else if (minusResult >= 2) score = "Win for " + _player1Name;
            else score = "Win for " + _player2Name;
            return score;
        }

        private string GetScoreForEquality()
        {
            if (_player1Score <= 2) return _possibleScores[_player1Score] + "-All";
            return "Deuce";
        }
    }
}

Nous avons un code plus clair et moins verbeux. Ce n’est ni la seule façon de procéder, ni la meilleure. Mais elle me paraît acceptable. Cette notion d’acceptabilité est extrêmement importante. Elle dépend des développeurs, de l’équipe, des normes de dev si elles existent… J’aurais pu aller plus loin en introduisant des constantes pour tous les entiers par exemple. Ainsi que des constantes pour toutes les chaînes de caractères en dur restantes. Mais je me suis seulement assuré qu’elles n’étaient jamais dupliquées. Ou des variables booléennes pour les if, afin de les rendre plus explicites. Mais j’ai jugé qu’avec un minimum de connaissance fonctionnelle et le nom de la méthode où je me situais, cela suffisait. J’aurais également pu introduire des commentaires. Ce que je n’ai pas fait pour exactement les mêmes raisons.

Le refactoring, tout comme le développement “standard”, est une activité subjective. On peut ne jamais s’arrêter. J’ai, par exemple, hésité à introduire une classe Player portant un nom et un score. Mais je pense que j’aurais introduit une complexité inutile qui n’apportait aucune clarté au code.

Attention. J’ai fait totalement abstraction dans cet article de toute référence aux tests unitaires. Le refactoring est une opération à risque. Même un renommage de variable peut poser problème, en cas de réflexion ou de binding, par exemple. Les tests unitaires doivent servir de harnais de sécurité. Dans cet exemple, j’ai une batterie complète de TU qui me garantissent le bon fonctionnement de mon application. Le plus souvent, une erreur de refactoring entraîne une erreur de compilation, mais pas systématiquement.

Ensuite, entraînez-vous. Les katas utilisés ici sont un excellent moyen de le faire. Apprenez et comprenez le fonctionnement de vos outils. Resharper, quant à lui, n’est pas une silver bullet. Il ne résoudra pas vos problèmes pour vous. En revanche, il met en valeur énormément de code améliorable et propose systématiquement une ou plusieurs solutions.

Dernière chose, il est très intéressant de noter que pour un même développeur, pour un comportement identique, le résultat final d’un refactoring peut être entièrement différent en fonction de la base initiale de code. Par exemple, dans ce kata, il y a 3 classes TennisGame qui ont un comportement identique, mais sont écrits différemment. J’ai un résultat différent après refactoring de ces 3 classes. Voilà, c’est tout pour cette fois. Peut-être qu’il y aura une suite à cette série, car la liste des types de refactorings est très longue. Nous verrons bien.