Refactoring: un exemple simple en Ruby

Bonjour,

Récemment, on m’a demandé « A quoi ça sert, le refactoring ? ». À cette question, ma réponse est toujours la même :

Une ligne de code, je l’écrit une fois, mais je la lit des dizaines de fois. Et peut-être que d’autres personnes la liront à leur tour des dizaines de fois. La logique veut donc qu’on juge la qualité d’un code à sa lecture.

Cet article est le premier d’une série consacrée au refactoring. Je prendrais des exemples concrets, tirés (ou adaptés) d’applications réelles et je montrerais comment en améliorer la lisibilité. La série ne sera pas centrée uniquement sur le langage Ruby. Il pourra y avoir de temps à autre des exemples en Java, Python, Php, etc.

L’exemple d’aujourd’hui est une classe très simple, qui explore la ligne de commande pour trouver quelle commande l’utilisateur souhaite lancer. Il est tiré de yabu, un utilitaire de sauvegarde pour linux.
Voici la classe CommandParser telle qu’elle existait:

# I find the user command in the command line.
class CommandParser

  def initialize args = ARGV
    @args = args
  end

  # The command 'backup' is a special case because it is the default command.
  def backup?
    result = command? "backup"
    if result == false
      result = true if @args.empty?
    end
    result
  end

  def recover?
    command? "recover"
  end

  def help?
    command? "help"
  end

  private

  def command? command
    @args.first == command
  end

end

# Embedded tests
puts "must be 'recover'" unless CommandParser.new(['recover']).recover?
puts "must be 'help'" unless CommandParser.new(['help']).help?
puts "must be 'backup'" unless CommandParser.new(['backup']).backup?
puts "must be 'backup' too" unless CommandParser.new([]).backup?

puts "must not be 'recover'" if CommandParser.new(['azerty']).recover?
puts "must not be 'recover' too" if CommandParser.new([]).recover?
puts "must not be 'help'" if CommandParser.new(['azerty']).help?
puts "must not be 'backup'" if CommandParser.new(['azerty']).backup?

On comprend facilement l’objectif et l’utilisation de la classe. Malgré tout, on peut faire mieux.

D’abord un mot sur les tests unitaires

Faire du refactoring sans une suite de tests unitaires est irréaliste. Pour la simplicité de l’exemple, j’ai écrit les tests à la suite de la classe, dans le même fichier. Il suffit de lancer le fichier ainsi:

  ruby before.rb

et de vérifier que la sortie est nulle. Dans la réalité, j’utilise le framework Rspec pour les tests de yabu.

Note: Si vous n’avez pas encore adopté la pratique du Test Driven Development, ou du Behavior Driven Development, il est peut-être temps de vous lancer…

C’est parti !

La partie qui me pose problème est la méthode backup?:

  # The command 'backup' is a special case because it is the default command.
  def backup?
    result = command? "backup"
    if result == false
      result = true if @args.empty?
    end
    result
  end

Cette méthode a beau être courte, sa logique est embrouillée et sa lecture demande un certain effort.

1 Tirer parti du langage

On tire bénéfice du mot-clé unless, qui est l’opposé du if, et qui permet de supprimer un test.

  # The command 'backup' is a special case because it is the default command.
  def backup?
    result = command? "backup"
    unless result
      result = true if @args.empty?
    end
    result
  end

2 Supprimer le code inutile

On se sert du fait que la méthode empty? renvoie déjà la valeur booléenne qu’on cherche. Un second test est supprimé.

  # The command 'backup' is a special case because it is the default command.
  def backup?
    result = command? "backup"
    unless result
      result = @args.empty?
    end
    result
  end

3 Améliorer l’algorithme

Maintenant, c’est l’algorithme qui est remis en cause. On s’aperçoit que si result contient false, il suffit de renvoyer directement la valeur de @args.empty?.

  # The command 'backup' is a special case because it is the default command.
  def backup?
    result = command? "backup"
    if result
      true
    else
      @args.empty?
    end
  end

4 Supprimer encore l’inutile

Comme le code s’est simplifié, la valeur intermédiaire result est devenue inutile.

  # The command 'backup' is a special case because it is the default command.
  def backup?
    if command? "backup"
      true
    else
      @args.empty?
    end
  end

On peut aussi préférer la syntaxe suivante:

  # The command 'backup' is a special case because it is the default command.
  def backup?
    return true if command? "backup"
    @args.empty?
  end

Personnellement, je n’ai pas de préférence pour l’une ou l’autre forme.

5 En finir avec les commentaires

Il est temps de s’occuper du commentaire. La plupart du temps, avoir un commentaire signifie qu’on a échoué à écrire un bon code. Le code devrait se lire comme une histoire. Pour cela, il faut qu’il ai du sens. Quand on donne du sens au code, le besoin de commentaire disparaît.

  def backup?
    return true if command? "backup"
    default_command?
  end

  def default_command?
    @args.empty?
  end

Articles en relation

Coco, code coverage pour ruby 1-9
Améliorer son code Ruby avec reek
3 trucs pour simplifier son code Ruby
Comment tester une méthode privée en Ruby

Publicités

, , ,

  1. #1 par Adrien Jarthon (@adrienjarthon) le 07/12/2011 - 13:23

    Bon exemple ^^ en tant qu’adepte du refactoring Ruby, j’aurai poussé jusqu’au:

    def backup?
    command?(‘backup’) or @args.empty?
    end

    Mais c’est peut être une préférence personnelle 😉

  2. #2 par Mathieu Fontaine (@5pidou) le 07/12/2011 - 14:57

    Oui j’allais dire la même chose… moi j’aurais écris :

    def backup?
    command?(‘backup’) || default_command?
    end

    qui à mon sens et beaucoup plus lisible que « return true if … »

  3. #3 par undeveloppeur le 04/01/2012 - 19:10

    @Adrien et @Mathieu, effectivement on peut pousser plus loin que je ne l’ai fait. Merci pour ces précisions et bonne année !

Laisser un commentaire

Entrez vos coordonnées ci-dessous ou cliquez sur une icône pour vous connecter:

Logo WordPress.com

Vous commentez à l'aide de votre compte WordPress.com. Déconnexion / Changer )

Image Twitter

Vous commentez à l'aide de votre compte Twitter. Déconnexion / Changer )

Photo Facebook

Vous commentez à l'aide de votre compte Facebook. Déconnexion / Changer )

Photo Google+

Vous commentez à l'aide de votre compte Google+. Déconnexion / Changer )

Connexion à %s

%d blogueurs aiment cette page :