-
-
Save jdeniau/0c3df8a498787dc30078 to your computer and use it in GitHub Desktop.
<?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; | |
} |
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
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.
Injection SQL
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
.
Dans le pire des cas, il aurait du utiliser quelque chose comme mysql_real_escape_string, et mieux, utiliser un paramètre PDO
Paramètre inutilisé
Ligne 9
$day_time_end
n'est jamais utilisé.
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).
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;
}
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é leif ($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.