Simplifiez vos conditions

Dans notre précédent article, nous avons examiné le typage en PHP. Voyons à présent quelques règles simples qui rendent le code plus lisible dans un contexte de conversion implicite, et plus particulièrement au sein des structures conditionnelles.

Mauvaise utilisation de count

Je rencontre régulièrement ce genre de test sur des array:

1
2
3
4
5
function foo($bar){
    if(count($bar)>0){
        ...
    }
}

En lisant ce code, on peut être amené à penser que la condition s’assure que le tableau n’est pas vide et que le traitement ne s’effectue que si le tableau contient au moins une valeur. En réalité, ce n’est pas exactement ce qui est fait. Si on veut être très précis:

  1. On compte le nombre de valeurs présentes dans le tableau.
  2. On s’assure que ce nombre est supérieur à 0.

Autrement dit, le code ne fait pas ce pour quoi il est destiné. Avant toute chose, le code compte le nombre de valeurs du tableau. Le fait que l’on passe la condition lorsque le tableau est vide n’est rien d’autre qu’un effet de bord, une sorte de hack, un détournement du code.

Bien sûr, ce genre d’écriture serait nécessaire si une condition n’acceptait que des boolean. Ce n’est pas le cas en PHP qui convertit les valeurs de manière implicite. Si on caste un array en boolean, il vaut false s’il est vide et true s’il contient au moins une valeur.

1
2
(bool) array(); //false
(bool) array(false); //true

La conversion implicite d’un tableau suffit à savoir s’il est vide.

1
2
3
4
5
function foo($bar){
    if($bar){
        ...
    }
}

Cette fois-ci, le code fait exactement ce que l’on attend de lui. L’écriture s’en trouve simplifiée, et, en éliminant ainsi du bruit superflu, le code devient enfin lisible.

Précisons également qu’il est inutile de procéder à un tel test en amont d’un foreach. Il est évident qu’un foreach ne réalise aucune boucle sur un tableau vide.

Mauvaise utilisation de is_array

Un autre cas typique de détournement de code peut arriver avec l’écriture suivante.

1
2
3
4
5
6
7
function foo($bar){
    if(is_array($bar)){    
        foreach($bar as $value){
            ...
        }
    }
}

La fonction is_array() sert uniquement à tester le type d’une valeur. Elle permet donc de traiter dynamiquement des valeurs selon leur type. A contrario, is_array() n’est pas destiné à se prémunir d’un mauvais comportement, comme par exemple l’utilisation par un foreach d’une valeur qui ne soit pas itérable.

La condition ci-dessus peut donc être bonne ou mauvaise, selon l’intention du codeur. Ce code peut être ambigü à la base, et seul en étant précis, on reste clair sur ses intentions. C’est pour cette raison (de ne pas induire le lecteur en erreur) qu’il est important, dans ce cas précis, de ne pas détourner la fonction is_array().

Test sur le type

Premier cas d’utilisation: La condition permet de traiter l’argument reçu comme une variable contenant potentiellement plusieurs types de valeurs possibles. Dans un tel cas, le code se tient car la fonction is_array() est correctement utilisée et permet de traiter dynamiquement plusieurs comportements différents.

Le code pourrait alors se traduire de la sorte:

1
2
3
4
5
6
7
8
function foo($bar){
    if(is_array($bar)){    
        //traitement dans le cas où $bar est un tableau
    }elseif(is_string($bar)){
        //traitement dans le cas où $bar est une string
    }
    ...
}

Par contre, il serait préférable de déléguer le traitement de chaque type à des sous-fonctions, si on ne désire pas outre-passer le niveau d’abstraction de cette fonction principale, tel que décrit par l’Oncle Bob.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
function foo($bar){
    if(is_array($bar)){    
        fooForArray($bar);
    }elseif(is_string($bar)){
        fooForString($bar);
    }
    ...
}

function fooForArray(array $bar){
    foreach($bar as $value){
        ...
    }
}

...

Ce genre de code arrive fréquemment si on travaille de manière récursive.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
function mixedToUpper($data){
    if(is_array($data)){
        return arrayToUpper($data);
    }
    if(is_string($data)){
        return strtoupper($data);
    }
    return $data;  
}

function arrayToUpper(array $data){
    foreach($data as $key => $value){
        $data[$key] = mixedToUpper($value);
    }
    return $data;
}

var_dump(mixedToUpper(array('a', array('a')))); // => les 'a' sont passés en MAJ

Se prémunir d’un mauvais comportement

Second cas d’utilisation: La condition sert uniquement à s’assurer que la fonction reçoit effectivement un tableau comme argument.

Le code pourrait alors se traduire de la sorte:

1
2
3
4
5
6
function foo($bar){
    if(is_array($bar)){    
        ...
    }
    throw new InvalidArgumentException('$bar must be an array.'):
}

Cela pourrait arriver si l’on veut se prémunir d’un mauvais comportement avant une boucle foreach par exemple.

1
2
3
4
5
6
7
function foo($bar){
    if(is_array($bar)){ //ce test sert uniquement à prévenir d'une erreur d'exécution dans le cas où $bar n'est pas un array
        foreach($bar as $value){
            ...
        }
    }  
}

Pourtant, cela rend le code beaucoup moins lisible. On rajoute une condition qu’il faudra à nouveau comprendre lorsque, quelque temps après, on essaiera de relire le code. Et souvent, on aura omis d’y jouxter un commentaire.

Un commentaire n’aurait toutefois pas été une excuse! Au contraire, on aurait eu un indice que le code n’est pas lisible. En général, si on doit rajouter des commentaires inline, c’est qu’il faut réécrire le code, pas le commenter.

Mais avant tout, le code n’est pas bon car la fonction is_array est détournée. On confond le rôle de cette fonction. On ne désire pas vraiment savoir s’il s’agit d’un array; on essaie juste de se prémunir d’une mauvaise utilisation du code par un autre développeur. Ce code est donc erroné d’un point de vue sémantique.

Si vous voulez que le code n’engendre pas de mauvais comportement, il vous suffit tout simplement d’utiliser le type hinting.

1
2
3
4
5
function foo(array $bar){
    foreach($bar as $value){
        ...
    }
}

A nouveau, le code est simplifié. Plus clair, plus court, plus juste. Et intérêt supplémentaire non négligeable, on gagne également en lisibilité au niveau de la signature de la fonction: pas besoin de rentrer dans le code pour savoir le type d’argument à passer.

Mauvaise utilisation de empty

Dans le même esprit, voici une écriture utilisant empty pour de mauvaises raisons.

1
2
3
4
5
function foo($bar){
    if(!empty($bar)){
        ...
    }
}

Que fait la fonction empty (peu importe le type, le comportement est toujours identique)? Deux choses:

  1. Elle teste l’existence d’une variable, à l’instar d’isset.
  2. Elle effectue une comparaison pour savoir si la valeur est équivalente à false. Cette comparaison est laxiste et implique donc une conversion implicite du type.

Autrement dit, par rapport à notre bout de code:

  1. Le premier test ne sert à rien, étant donné qu’on est certain de l’existence du paramètre de la fonction.
  2. Le deuxième test ne sert à rien, étant donné que la condition if effectue d’office une conversion vers un bool.

Autrement dit donc, empty ne sert à rien, et il suffit d’écrire simplement ceci:

1
2
3
4
5
function foo($bar){
    if($bar){
        ...
    }
}

A nouveau, simplicité signifie lisibilité.

Et lorsque l’on sait ça, j’avoue ne pas très bien avoir compris la raison pour laquelle PHP 5.5 permet désormais d’utiliser empty sur le résultat d’une expression…

Utilisez la comparaison stricte

Si vous pouvez, écrivez des comparaisons strictes! Préférez la comparaison « === » ou « !== » dès que possible.

Vous excluez ainsi la possibilité d’une conversion implicite. Le code est plus facile à comprendre, car il ne fait qu’une seule chose. Celui qui vous relit ne devra pas se poser la question de l’interprétation des types. La comparaison laxiste ne sert à rien d’autre qu’à engendrer une confusion dans le code.

Par ailleurs, vous échapperez aussi à quelques erreurs vicieuses auxquelles on ne pense pas en codant…

1
2
3
4
5
$var = 1
if($var == 'abc'){
    //je ne passe pas
    //en effet, le int est casté en string => '1' === 'abc'
}
1
2
3
4
5
$var = true;
if($var == 'abc'){
    //je passe
    //en effet, la string est castée en bool => true === true
}

Mieux vaut donc faire soit

1
2
if($var){
}

soit

1
2
if($var === 'abc'){
}

Et si vous utilisez une comparaison laxiste, expliquez-en la raison dans un commentaire adjacent.

Conclusion

Simplifier les conditions permet avant tout de gagner en lisibilité. En général, en informatique, plus la solution est simple, meilleure elle sera.

Et par la même occasion vous éviterez de hacker PHP et de détourner des fonctions. Votre code sera plus juste.

7 réflexions au sujet de « Simplifiez vos conditions »

  1. Merci pour l’article !
    Tant qu’on est dans les bonnes pratiques de structures conditionnelles, je préfère écrire :
    if(‘abc’ == $var){
    }

    que

    if($var == ‘abc’){
    }

    qui permet d’éviter des comportements inattendus en cas de faute de frappe (en effet, $var = ‘abc’ passera, là où ‘abc’ = $var nous remontera une erreur).

  2. bon article ! ça fait longtemps que je vois ce genre de choses en php.
    juste petite typo :
    à un moment tu écris « il faut false s’il est vide et true s’il contient au moins une valeur. ». je pense que tu voulais dire « il vaut ».

    aussi concernant is_array() et foreach(), j’ai peut-être mal compris mais tu ne parles pas du fait que foreach sert avant tout à itérer, que ce soit sur un tableau ou un objet itérable. du coup si on teste systématiquement avec is_array(), au moindre changement (array vers objet itérable), le code est cassé.

  3. Très intéressant Raph. Vive la simplicité du code.
    C’est drôle, mais j’ai l’impression de reconnaître une discussion que nous avons eu sur empty au retour d’une expression.

Laisser un commentaire

Votre adresse de messagerie ne sera pas publiée. Les champs obligatoires sont indiqués avec *

Vous pouvez utiliser ces balises et attributs HTML : <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>