« retourner à la page principale du blog
Bon code, Bad code
Il n’y a pas si longtemps, j’ai eu à intégrer du code PHP qu’un autre programmeur avait déjà fait pour un autre site. Le client tenait à ce que j’utilise son « module » de nouvelles en pensant sauver de l’argent. La première fois que j’ai eu à intégrer ledit module, ç’a effectivement été plutôt rapide étant donné que le site était fait dans le même pattern que le site d’origine. Par contre, la deuxième fois, c’était une toute autre chose!
Du code non-réutilisable
J’espère ne rendre personne triste en publiant cet article, mais je vais vous faire un petit copier-coller d’une fonction pour illustrer mes points.
<?php
// _______________________________________________________________________
// Display news list
function fct_display_news_list(){
// Search
if(isset($_GET['search']) && !empty($_GET['search'])){
$query = " WHERE title LIKE CONVERT(_utf8 '%".$_GET['search']."%' USING latin1) OR content LIKE CONVERT(_utf8 '%".$_GET['search']."%' USING latin1)";
}else{
if(isset($_GET['date']) && ereg("^[0-9]{4}\-[0-9]{2}$", $_GET['date'])){
$arr = explode("-", $_GET['date']);
$date_start = mktime(0, 0, 0, $arr[1], 1, $arr[0]);
$date_end = mktime(0, 0, 0, $arr[1] + 1, 1, $arr[0]);
$query = " WHERE date_news>=".$date_start." AND date_news<".$date_end;
}
}
if(!isset($query)){
$query = " ORDER BY date_news DESC LIMIT 3";
}else{
$query .= " ORDER BY date_news DESC";
}
$query = "SELECT * FROM juris_news".$query;
$result = mysql_query($query) or die(mysql_error());
if(mysql_num_rows($result) > 0){
for($i = 1; $row = mysql_fetch_array($result); $i++){
?>
<h3 first"); } ?>"><?= $row['title']; ?></h3>
<p><?= date("Y-m-d", $row['date_news']); ?></p>
<p><?= $row['intro']; ?></p>
<p><a href="actualites.php?id=<?= $row['id']; ?>" onmouseout="MM_swapImgRestore()" onmouseover="MM_swapImage('news<?= $i; ?>','','medias/img_c_btn_savoir_rl.gif',0)"><img src="medias/img_c_btn_savoir_up.gif" alt="En savoir plus" name="news<?= $i; ?>" id="news<?= $i; ?>" /></a></p>
<?php
}
}else{
echo("<p>Aucune actualité trouvée.</p>");
}
}
Il s’agit ici de la première fonction d’une dizaine de fonctions similaires qui constituent le fichier.
Premier péché: mélanger la logique avec la présentation
La principale force de PHP est aussi sa principale faiblesse: il est très facile de mélanger du PHP avec du HTML. On peut avoir une page entière en HTML et y insérer très facilement un peu de PHP. On peut par contre aussi avoir du PHP et y insérer du HTML. Le problème avec cette fonction, c’est qu’elle génère (et affiche) du HTML.
On peut voir très facilement que le code HTML affiché n’est pas réutilisable. Il y a des appels à des fonctions JavaScript et des images dont le chemin d’accès est hard coded. Par exemple, la fonction affiche:
// mauvais
<p><a href="actualites.php?id=<?= $row['id']; ?>" onmouseout="MM_swapImgRestore()" onmouseover="MM_swapImage('news<?= $i; ?>','','medias/img_c_btn_savoir_rl.gif',0)"><img src="medias/img_c_btn_savoir_up.gif" alt="En savoir plus" name="news<?= $i; ?>" id="news<?= $i; ?>" /></a></p>Il est fort peu probablement que j’aie à utiliser les mêmes fonctions JavaScript et que j’utilise les mêmes images. Aussi, le markup HTML généré ne conviendra générallement pas aux besoins d’un autre site. Par exemple, je doute que <h3 class="blue"> puissent être réutilisé.
La meilleur façon aurait été de simplement arrêter la fonction à la fin de la requête SQL. La fonction aurait simplement pu retourner un array, un objet ou un itérateur. De cette manière, on sépare la logique de la présentation. Par exemple, si la fonction retournait un array $nouvelles, on pourrait l’intégrer dans la page directement.
<!-- Mieux -->
<!-- Markup de ce qui vient avant dans la page ici... -->
<div id="nouvelles">
<h2>Nouvelles</h2>
<?php if (!empty($nouvelles)) : ?>
<?php foreach ($nouvelles as $nouvelle) : ?>
<p class="date"><?= date("Y-m-d", $nouvelle['date_news']) ?></p>
<p><?= $nouvelle['intro'] ?></p>
<p><a href="actualites.php?id=<?= $nouvelle['id']; ?>" class="more">En savoir plus</a></p>
<?php endforeach ?>
<?php else : ?>
<p>Aucune nouvelle</p>
<?php endif ?>
</div>
<!-- Suite du site ici... -->
<div id="sidebar">Deuxième péché: La fonction n’accepte aucun paramètre
De la façon dont la fonction est codée, on n’a aucun moyen externe (lire: aucun paramètre) d’affecter son déroulement. Si $_GET['search'] contient quelque chose, une recherche sera faite, sinon, les trois dernières nouvelles sont affichées.
Il serait bien de pouvoir appeler la fonction en lui demandant un certain nombre de nouvelles. Je ne veux pas avoir toujours les trois dernières nouvelles dans tous les sites que je dois faire.
Aussi, si je veux faire une recherche, je ne veux pas nécessairement utiliser $_GET['search'] comme chaine à chercher.
Le prototype de cette fonction aurait plutôt dû ressembler à ceci:
function fct_display_news_list($quantity = 3, $search = NULL)De cette manière, on aurait pu avoir des comportements par défaut si on ne passe aucun argument tout en ayant un certain contrôle en cas de besoin. Si je voulais à ce moment utiliser la fonction en utilisant la valeur de $_GET['search'] comme clé de recherche, j’aurais pu l’appeler ainsi:
fct_display_news(3, $_GET['search'])Troisième péché: la fonction fait trop de choses
La fonction « raboute » des morceaux de requêtes SQL en créant la fin de la requête ("WHERE ...") en premier. Je trouve étrange d’avoir commencé par la fin sans raison apparente. Ça ne fait que complexifier le code.
En plus, pourquoi ne pas avoir simplement séparé la fonction en deux fonctions? Chaque fonction aurait été beaucoup plus claire et facile à maintenir. Exemple:
// mieux
function get_news($quantity = 3, $search = NULL){
if ($search == NULL)
return get_latest_news($quantity);
else // $search != NULL
return get_searched_news($quantity, $search);
}
function get_lastest_news($quantity = 3){
// fonction qui retourne les $quantity dernières nouvelles
// ...
}
function get_searched_news($quantity = 3, $search = NULL){
// fonction qui retourne les $quantity dernières nouvelles trouvées selon la clé de recherche
// ...
}Si on regarde la liste des fonctions du fichier, on se rend vite compte qu’une approche orienté objet aurait très bien fait la job. Une classe News aurait grandement simplifié le tout.
Quatrième péché: des noms qui ne veulent rien dire
Si on retourne dans le code au début de l’article, on peut voir qu’il y a plusieurs noms encombrés d’abréviations superflues ou qui ne veulent rien dire aux yeux de la prochaine personne qui touche le code.
Est-ce que j’ai vraiment besoin de spécifier dans le nom d’une fonction qu’il s’agit d’une fonction en y préfixant fct? Aussi, un fichier nommé img_c_btn_savoir_up.gif: un fichier gif est une image, pas besoin de préfixer img à son nom. Dans le même nom de fichier, que veut dire le « c« ?
Une fonction devrait avoir un nom qui permet de savoir ce qu’elle fait tout en étant concis. display_news_list aurait été suffisant.
Cinquième péché: être vulnérable aux injections SQL
Étant donné que la fonction n’accepte aucun paramètre, le programmeur a utilisé la valeur de $_GET['search'] directement dans sa fonction. Le problème que je veux souligner ici c’est la manière dont il l’a utilisée. Il l’a mis directement dans une requête sans la filtrer:
// mauvais
$query = " WHERE title LIKE CONVERT(_utf8 '%".$_GET['search']."%' USING latin1) OR content LIKE CONVERT(_utf8 '%".$_GET['search']."%' USING latin1)";
En insérant directement des valeurs fournies par l’utilisateur (ici, dans la barre d’adresse) sans les avoir filtrés au préalable est une mauvaise pratique car elle ouvre très grand les portes aux attaques par injection SQL.
En conclusion
Quand on programme, c’est important de toujours penser à la réutilisabilité du code. Il faut bien sûr parfois faire du code « quick and dirty« , mais les choses importantes que je viens d’énumérer ne sont pas compliqués à respecter. D’ailleurs, le programmeur aurait en fait sauvé du temps s’il avait fait du code réutilisable, car il a eu à reprogrammer un autre ensemble complet de fonctions pour le côté administrateur. S’il avait dès le départ fait une nouvelle classe ou simplement mieux conçu ses fonctions, il aurait facilement pu sauver beaucoup de temps en réutilisant lui-même son propre code à l’intérieur du même projet.
27 novembre 2009 à 6:14
Bon article! Les bonnes pratiques font les bons programmeurs!