Navigation Menu

Skip to content

Instantly share code, notes, and snippets.

@jdeniau
Created August 20, 2014 07:09
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save jdeniau/0c3df8a498787dc30078 to your computer and use it in GitHub Desktop.
Save jdeniau/0c3df8a498787dc30078 to your computer and use it in GitHub Desktop.
Classe model
<?php
function search($date = null, $departure = null, $arrival = null)
{
if ($departure == null || $arrival == null)
return null;
if ($date != null) {
$day_time = strtotime($date);
$day_time_end = $day_time + 86400;
$sql = 'SELECT * FROM `ride` as r, `user` as u
WHERE r.`user_id` = u.`id`
AND r.`towndeparture` LIKE "%'.urldecode($departure).'%"
AND r.`townarrival` LIKE "%'.urldecode($arrival).'%"
AND r.`timestamp` >= "'.$day_time.'"
GROUP BY `id_ride`';
}
else
$sql = 'SELECT * FROM `ride` as r, `user` as u
WHERE r.`user_id` = u.`id`
AND r.`towndeparture` LIKE "%'.urldecode($departure).'%"
AND r.`townarrival` LIKE "%'.urldecode($arrival).'%"
AND r.`timestamp` > NOW() + 3600
GROUP BY `id_ride` ;';
$query = $this->db->query($sql);
if ($query->num_rows() > 1) {
$result = array();
foreach ($query->result() as $row)
$result[] = $row;
return $result;
}
if ($query->num_rows() == 1)
return array($query->row());
return null;
}
@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Paramètre $date

Ligne 7
Le paramètre $date est plus ou moins optionnel, et il a un default à NOW() + 3600.
Du coup il aurait été plus judicieux déjà, de le mettre à la fin des trois paramètre, et d'avoir $departure et $arrival en obligatoire : ce qui aurait évité le if ($departure == null/* ... */) return null;

Architecture

Quoi qu'il en soit, en terme d'architecture, ce n'est je pense pas au modèle de savoir quoi faire si la date est vide. C'est de la logique métier, c'est donc plus du côté du controller de faire ce test.

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Duplication de code

Ligne 17
Le else ne sert que dans le cas $date == null et ne change que la valeur de date, il aurait été plus judicieux de faire quelque chose comme :

if ($date === null) {
    $date = strtotime('+1 hour');
} else {
    $date = strtotime($date);
}

Avec une seule fois la requête

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

if > 1, else if 1 else...

Ligne 25
A cet endroit, il fait quelque chose d'assez bizarre. Traduit en français, ça donnerait:

Si j'achète plusieurs fruits: Prends un panier, mets les fruits un par un dedans et passe a la caisse.
Si j'achète un seul fruit: met le fruit dans un panier, et passe à la caisse.
Sinon passe à la caisse

La deuxième ligne ne sert en fait à rien, puisqu'elle ferait la même chose que la première.

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Injection SQL

Ligne 13

Je n'ai pas l'intégralité du code, étant donné que ce sont des paramètres qui sont passés, cela dit, le fait qu'il fasse un urldecode me laisse penser que ça vient d'un paramètre GET ou POST.

⚠️ Cela laisse libre recours à une injection SQL très basique.

Dans le pire des cas, il aurait du utiliser quelque chose comme mysql_real_escape_string, et mieux, utiliser un paramètre PDO

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Paramètre inutilisé

Ligne 9
$day_time_end n'est jamais utilisé.

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Return null

Ligne 33
C'est un peu étrange de retourner null lorsqu'il n'y a pas de résultat, plutôt que tableau vide, qui fait plus de sens (c'est bien une liste de 0 départ).

@jdeniau
Copy link
Author

jdeniau commented Aug 20, 2014

Refactoring

Voila comment je l'aurais fais moi:

<?php
function search($departure, $arrival, $date = null)
{
    if (!$date) {
        $dayTime = time() + 3600;
        } else {
        $dayTime = strtotime($date);
    }

    $sql = 'SELECT * FROM `ride` as r, `user` as u
        WHERE r.`user_id` = u.`id`
        AND r.`towndeparture` LIKE "%:departure%"
        AND r.`townarrival` LIKE "%:arrival%"
        AND r.`timestamp` >= ":dayTime"
        GROUP BY `id_ride`';

    $params = ['departure' => $departure, 'arrival' => $arrival, 'dayTime' => $dayTime];
    $preparedQuery = $this->db->prepareQuery($sql, $params); // pseudo code
    $query = $this->db->query($preparedQuery);

    $result = [];
    foreach ($query->result() as $row) {
        $result[] = $row;
    }
    return $result;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment