Aujourd’hui, nouvelle fournée de perles.
Au programme,du commentaire « utile », une existante native réécrite (et bugée), et une condition qui ne fait pas ce qu’elle devrait 🙂
Le « if » de trop…Ou le « else » de trop peu?
|
1 2 3 4 |
function getFlow() { if (isset($this->datas["flow_id"])) if (isset($this->datas["flow_id"])) return $this->datas["flow_id"]; else return 0; } |
Review de code 1: Pourquoi deux fois exactement le même if l’un à la suite de l’autre? Peut-être un copier coller.
Review de code 2: Si « flow_id » n’est pas setté, la méthode ne renvoit pas « 0 » (zéro), comme le laisse entre la seconde ligne. En effet, si l’on indente son code correctement, voici ce que cela donne:
|
1 2 3 4 5 6 7 8 9 |
function getFlow() { if (isset($this->datas["flow_id"])){ if (isset($this->datas["flow_id"])){ return $this->datas["flow_id"]; }else{ return 0; } } } |
En ajoutant les accolades, c’est encore plus clair: Si « flow_id » n’existe pas, la méthode ne renverra rien.
Le commentaire pour les aveugles
Juste une petite anecdote ici, rien de bien méchant.
|
1 2 3 4 |
// REMARK: if block = 3 if ($block_id == 3) { //du code... } |
Review de code: Merci pour ce commentaire qui m’a bien aidé à comprendre la condition.
L’ « array_unique » du pauvre
|
1 2 3 4 5 6 7 8 9 10 11 12 |
function compare(array $values) { $valuesList = array(); foreach ($values as $value) { if (!empty($valuesList)) { if (in_array($value,$valuesList)) { return false; } } $valuesList[] = $valuesList; } return true; } |
Petite explication:
- La fonction reçoit un tableau de valeurs (toutes des chaines de caractères).
- On prépare un autre tableau de backup ($valuesList), qui va sans doute servir à transvaser les valeurs une à une… Enfin c’est ce que l’on croit.
- On boucle sur les valeurs,
- Pour chaque valeur, si le tableau de backup n’est pas vide, on vérifie que la valeur en cours n’est pas déjà dedans. Si c’est le cas, on renvoi false
- Si la valeur ne se trouve pas dans le tableau de backup, on l’insère dans lui même. Et ça c’est beau. On va donc avoir un tableau qui va contenir uniquement un autre tableau, et ainsi de suite… Et ce autant de fois qu’on a de valeurs.
La méthode renvoi donc TOUJOURS true. et « $valueList » vaut ceci après 2 itérations:
|
1 2 3 4 5 |
array( array( array() ) ) |
Review de code 1: Nom de la méthode pas très explicite
Review de code 2: Renvoi toujours true
Review de code 3: Idée d’amélioration en une ligne (au cas où): return array_unique($values) === $values
Review de code 4: Dommage que ton test unitaire soit commenté,… Mais je comprends mieux pourquoi 🙂
A bientôt pour de nouvelles aventures. J’arrive avec plein d’articles sur les patterns. « Y’a plus qu’à »