Présentation de quelques code smell en C++

De part son histoire, le C++ hérite de certaines caractéristique du C. Les pointeurs en font partie et en particulier les pointeurs de type void*. Void est un type qui signifie rien et un pointeur de type void* est un pointeur qui pointe sur rien mais surtout sur tout. En effet, ces pointeurs sont appelés des pointeurs universels car n'importe quel pointeur (autres que ceux sur une fonction) peut être converti en void*. Exemple:

    int i=5;
    void* p=&i; // compile sans problème.

Le C utilisait les pointeurs de type void* afin de simuler le polymorphisme via des casts (implicites ou non)multiples. C'est d'ailleurs pour cela qu'en C++ le cast implicte void* vers T* a été interdit. De plus, le C++ dispose d'outils pour faire du polymorphisme, il n'a plus besoin de l'émuler. Dans ce cadre les pointeurs de type void* devraient être banni. En pratique, il ne faut jamais de fonction utilisant (c'est-à-dire renvoyant ou prenant) des pointeurs de type void*. Si vous devez utiliser du code existant qui fait usage de void*, il faut en premier lieu renforcer en le typage en encapsulant le code qui utilise des void*.

Les casts C-like sont des casts hérités du C. Ils correspondent à quelque chose du type T2 var2= (T1)exp. Ces cast sont permissifs et peu expressifs. Ils autorisent des choses légales selon le langage mais fausses. Exemple:

// class type-casting
#include <iostream>
using namespace std;

class CDummy {
    float i,j;
};

class CAddition {
	int x,y;
  public:
	CAddition (int a, int b) { x=a; y=b; }
	int result() { return x+y;}
};

int main () {
  CDummy d;
  CAddition * padd=(CAddition*) &d;
  cout << padd->result();
  return 0;
}

En effet, un cast C-like est éffectué sans aucune vérification de type. C'est ce laxisme qui autorise ce genre de code. Il faut mieux à la place utiliser les cast du C++ (static_cast, const_cast, dynamic_cast et reinterpret_cast). Nous reviendrons sur les deux derniers plus tard.

Le reinterpret_cast est un cast qui force le compilateur à lire la valeur d'un objet et à la considérer comme d'un autre type. Faire ceci suppose de savoir comment va être représenté en mémoire l'objet de dépard, donc rendre son code non portable. De plus, il supprime toute vérification de type que pourait faire le compilateur ce qui est une mauvaise chose car quand le typage diminue, la robustesse du programme aussi. N'oubliez pas: "If you lie to the compiler, it will get its revenge." (Henry Spencer)

Le dyanmic_cast est un cast qui permet de downcaster un objet. Autrement dit, quand une classe B hérite d'une classe A et qu'un pointeur ou une référence de type A pointe sur un objet de type B, il permet de transformer la référence/le pointeur en objet de type B. Exemple:

class A{
public:
  virtual ~A(){}
};

class B: public A
{
};

int main()
{
 A* a=new B;
 //B* b1=a; // ne compile pas.
 B* b2=dynamic_cast<B*>(a); //compile
 delete a;
}

En cas d'erreur lors de la tentative de cast, dynamic_cast renvoie un pointeur NULL si on tente de convertir un pointeur ou lance une exception pour les références. Ce système est utilisé afin de déterminer quel est le type réel de l'objet. Sauf que si vous manipulez un objet par un de ses parent, ce n'est pas sans raison: vous utilisez sans doute le polymorphisme dynamique et déterminer le type réel de l'objet va briser ce polymorphisme. Souvent, cette détermination peut être remplacée par une fonction virtuelle, une refactorisation, ou encore un pattern visiteur.

Les objets en C++ ont deux principales sémantiques: soit ils ont une sémantique d'entité, soit de valeur. On dit d'une classe qu'elle a une sémantique de valeur si deux objets situés à des adresses différentes, mais au contenu identique, sont considérés égaux. A l'inverse, une classe a une sémantique d'entité si toutes les instances de cette classe sont nécessairement deux à deux distinctes, même si tous les champs de ces instances sont égaux.

La première catégorie de classe est copiable. Cela signifie qu'elle dispose d'un opérateur d'affectation, d'un constructeur de copie et son destructeur est publique et non virtuel. L'autre type de classe n'est pas copiable ni affectable et son destructeur est souvent virtuel. Faire un mélange des deux va ammener à des mélanges imposible à gérer et prouve un problème de conception.

La plus part des gens pensent qu'en mettant des setters/getters dans une classe, il respecte l'encapsulation. Et bien, non, les setter/getter ne respectent pas l'encapsulation et sont à ce titre un code smell. Essayons de comprendre pourquoi.

Mettre des setters/getters dans une classe revient à exposer publiquement la mécanique interne de la classe. Or un des fondement de la POO est l'abstraction. Nous disposons d'un objet qui offre des services via son interface publique, nous ne savons par comment marche l'objet mais nous avons des garanties sur les résultats. Avec la POO, on pense en terme de service. Avec des getters/setters, on rend le code instable (une modification et ca peut être 25 points de code à modifier) et on ne pense plus en terme de service mais en terme de donnés. Ce n'est pas de l'encapsulation et donc pas de la POO.

Une règles fondamentale en programmation objet est qu'une classe n'a qu'un seul rôle et qu'une fonction ne fait qu'une seule chose. Cette règle assure d'avoir des objets simples, avec un couplage minimal entre eux ce qui permet d'augmenter le taux de réutilisation du code. Dans cette optique, si une fonction est trop longue , dispose de trop de niveaux d'indentation ou si une classe a trop de variables membres, c'est un signe qu'elle fait trop de choses et que la conception est à revoir. On ne peut pas fixer de valeur numérique pour dire quand c'est trop mais sachez que certains exercices demandent de limiter les fonctions à 50 lignes et à 2 niveaux d'indentation. En prenant un exemple (un peu extrême) et en restant dans cette optique, une classe qui gère une image et qui permet d'enregistrer l'image fait trop de chose.

Le copier/coller dans un code est une des erreurs les plus graves. En effet, tout code copié/collé qui fait un traitement est factorisable dans une fonction. Cette factorisation allège le projet, permet une meilleure robustesse et éviter devoir faire évoluer plusieurs fois le même code. Lorsqu'il s'agit d'un copier coller pour générer des surcharges, le code est factorisable à l'aide de boost::preprocessor.

Dans le domaine, les constantes magiques sont à proscrire. Les constantes magiques sont des chiffres écrits en dur dans le code. Par exemple:

if(x<250 && y > 120)
{
//faire quelque chose
}
else
{
//faire autre chose 
}

Que représente 250 et 120 ? Nous disposons d'aucun indices pour nous aider. Au mieux les valeurs sont documentées quelque part, au pire il faut devenier pourquoi cette valeur et non 42 ou 21. De plus, si ces valeurs sont utilisées N fois, faire évoluer une valeur va demander à modifier N fois le code. A la place, on aurait pu utiliser quelque chose comme:

static const int POSITION_X_MAX=250;
static const int POSITION_Y_MIN=120;
if(x<POSITION_X_MAX && y > POSITION_Y_MIN)
{
//faire quelque chose
}
else
{
//faire autre chose 
}

Ici, nous disposons du nom des constantes pour nous aider (le code est plus expressif), les modifications sont centralisées en un endroit et se propagent dans tous le code sans intervention.

Les fonctions variadiques sont un reliquat du C. Ce sont les fonctions qui utilisent ... dans leurs arguments afin de pouvoir en passer n'importe combien (comme printf). Ces fonctions ont un gros défaut car elle oblige le programmeur à se répéter. En effet, il doit d'abord passer la variable puis spécifier la manière de l'utiliser/afficher. Or cette manière est contenue en elle même dans la variable. Il ne faudrait pas avoir à la répéter. De plus, il suffit de changer le type de la variable pour que tous les appels de fonctions variadiques soit à changer ce qui est une source très importante de bug. Ces fonctions ne respecte le DRY ("don't repeat yourself"). Exemple d'une' fonction variadique (à éliminer en C++)

#include <cstdio>

int main(int argc, char const *argv[])
{
    int i=2;
    double d=5;
    const char* c="Ca va planter";

    printf("%ld/%lf/%s",i,d,c);
    return 0;
}

Il suffit de changer ld en lf ou le lf en ld pour que le code plante sur mon ordinateur.

Il est bien connu que si une erreur a lieu lorsque new tente d'allouer de la mémoire, une exception est lancée. Une pensée simpliste consiste à se dire qu'il suffit de rattraper l'exception et nettoyer pour que le programme se finisse bien. Exemple (à ne pas faire)

try
{
   int *t= new int[100000]; // je mets ici un nombre volontairement grand. Peut échouer comme non
}
catch(const std::bad_alloc& e)
{
   std::cout<<"manque de memoire"<<std::endl;
   throw; //on relance l'exception pour dire que l'allocation a échoué.
}

//on utilise t ...

//destruction
delete[] t;

Ce code semble bien au niveau de la mémoire. Pourtant, c'est une calamité. En effet, s'il faut gérer cela à la main pour chaque allocation, le code va vite être imbuvable. De plus, imaginons que l'allocation se passe bien mais que plus tard, l'utilisation d'une fonction lance une exception. Exemple

try
{
   int *t= new int[100000]; // je mets ici un nombre volontairement grand. Peut échouer comme non
}
catch(const std::bad_alloc& e)
{
   std::cout<<"manque de memoire"<<std::endl;
   throw; //on relance l'exception pour dire que l'allocation a échoué.
}

foo(t); //ici t lance une exception.

//destruction
delete[] t;

Comment libérer la mémoire ? C'est impossible de retomber correctement sur ses pieds avec une allocation dynamique explicite, même si un gestionnaire d'exception est placé. C'est pour cela qu'il faut placer toutes les ressources allouées avec new/new[] dans enveloppes RAII (comme boost::shared_ptr pour new ou utiliser un std::vector à la place de new[]).

Les macro sont des opérations réalisées par le pré-processeur. Elles ont été héritées du C qui les utilisait pour beaucoup de chose. Le C++ les utilise encore dans le domaine de l'inclusion de fichier, dans le domaine de la méta-programmation. Par contre, en C++ il ne faut plus utiliser de macros là où une fonction inline peut la remplacer. En effet, les macros sont du texte remplacé par un autre texte. Elles plus aucune existence après passage du pré-processeur, n'ont aucun typage et peuvent avoir des effets de bords. Exemple

#include <iostream>

#define FOO(a) (a)+(a)

inline int foo(int i) 
{
return i+i;
}

int main(int argc, char const *argv[])
{
    int i=2,j=2;
    std::cout<<FOO(2)<<std::endl;
    std::cout<<FOO(i)<<std::endl;
    std::cout<<FOO(i++)<<std::endl;
    std::cout<<FOO(++j)<<std::endl;

    std::cout<<"===="<<std::endl;

    i=2;j=2;
    std::cout<<foo(2)<<std::endl;
    std::cout<<foo(i)<<std::endl;
    std::cout<<foo(i++)<<std::endl;
    std::cout<<foo(++j)<<std::endl;
    return 0;
}

L'utilisation de FOO(++j) provoque une double pré-incrémentation de j, ce qui n'était pas voulu alors que l'appel à la fonction foo ne cause aucun effet de bord. C'est ce genre de bugs subtils qui doivent être évités en bannissant les macros quand elles ont un rôles de fonction inline.

Le syndrome du NIH (ou encore not invented here) est un syndrome chronique qui touche un certain nombre de programmeurs. Les symptômes sont assez éloquent: les programmeurs atteint de NIH codent des choses qui ont déjà été codées sous prétexte que cela n'a pas été codé par eux ou l'équipe. Cela peut aller des classes de la STL à des parseurs divers en passant par une myriade d'autres classes. Il y a quelques cas où recoder est obligatoire car les solutions ne sont pas efficaces. Mais dans la très grande majorité des cas, ces développeurs vont recoder des classes déjà faites, testées et validées. C'est une perte de temps, d'argent et d'énergie, 3 choses qui auraient pus être utilisées sur des points vraiment importants du projet. En résumé, même si le code recodé est de bonne qualité (ce qui est assez rare), c'est de mauvaise augure.

Dans cette même optique, la surcharge de new/new[] et delete/delete[] à des fins de performances est aussi mauvais signe car il y a de forte chance pour que les programmeurs précédents aient négligé certaines parties du code qui auraient pu être améliorées.

Le C++ permet (et impose dans certains cas) la surcharge des opérateurs. Cette surcharge est un outil puissant mais qui doit être manié avec précaution. Dans ce cadre, un certain nombre de "bonnes pratiques" sont à respecter. Prenons un exemple.

Le premier est celui de l'opérateur d'affectation. la manière classique d'écrire l'opérateur d'affection est la suivante:

class T
{
  Objet* ptr;
public:
T& T::operator=(const T& t)
{
  if(this != &t) //on se protège du cas de l'auto-affectation'
  {
    delete ptr;
    ptr=new Objet(*(t.ptr));
  }

 return *this;
}

};

Le code semble beau et sécurisé. Mais que se passe t'il si la création du nouvel objet lance une exception ? Tout s'écroule. Pour parer à ce problème, l'idiome copy-and-swap est conseillé à la place. Exemple

class T
{
Objet* ptr;
public:
T& T::operator=(const T& t)
{
  if(this != &t) //gain de performance en cas d'auto affectation'
  {
     T(t).swap(*this);
  }

 return *this;
}

 void swap (T &s) throw ();

};

Cette pratique est bien plus claire et sécurisée. De la même manière, trouver des opérateur+ qui sont mal définis (à partir d'une copie du code de l'opérateur + et non en l'utilisant) sont des signes que certains bonnes pratiques n'étaient pas connues et sont avant-coureur de problèmes similaires dans le code.