Mon code est-il bien codé ?

Avatar du membre
Modérateur PHPfrance
Modérateur PHPfrance | 8758 Messages

02 oct. 2014, 22:43

Tu veux dire que je peux faire ça dans la page du formulaire : login($email, $mdp); ?
non comme je t'ai indiquer utilise login($_POST['email'], $_POST['mdp']); ?
je pense que c'est une étourderie qui est resté parce qu'avec le temps on ne vois plus ce genre de chose à force de travailler sur son code (comme le nez au milieux de la figure ... ;) )
la fonction login corrigée (de ce point de vu la), il y a vraiment peux de différence.
<?php
function login($email,$mdp){
        if(!empty($email) && !empty($mdp)){
                if(preg_match("!^[a-z0-9._-]+@[a-z0-9._-]{2,}\.[a-z]{2,4}$!",$email)){
                        $req = Bdd::connect() -> prepare('SELECT id, pseudo, email, mdp FROM users WHERE email = :email AND mdp = :mdp');
                        $req -> execute(array('email' => $email,'mdp' => $mdp));
                        $donnees = $req -> fetch();
                        if(!$donnees){
                                echo '<span class="error">Vos identifiants sont incorrects</span>';
                        } else {
                                $_SESSION['id'] = $donnees['id'];
                                $_SESSION['pseudo'] = $donnees['pseudo'];
                                header('location: home');
                        }
                        $req -> closeCursor();
                } else {
                        echo '<span class="error">Veuillez entrer une adresse électronique valide</span>';
                }
        } else {
                echo '<span class="error">Veuillez remplir tous les champs</span>';
        }
}
pour la classe Bdd.
première chose, un objet doit être "étanche" au reste du monde. donc pas de constante globale déclarée autre par (même si je comprend pourquoi tu le fait je vais d'ailleurs les laisser par la suite).
Ensuite il te faut le concept de singleton (un peu détourné dans ce cas).
le but est d'avoir une seule instance de pdo pour ta base.
<?php

class Bdd {
    private static $connection;

    public static function connect() {
        if (is_null(self::$connection)) {
            try {
                self::$connection = new PDO(DNS, USER, PASS, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'));
                self::$connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
            } catch (Exception $e) {
                die('Échec lors de la connexion : ' . $e->getMessage());
            }
        }
        return self::$connection;
    }
}
du coup tu crée la connexion que la première fois et ensuite tu retourne toujours la même ;)

Il serait intéressant de virer le die et de traiter correctement l'erreur plutôt que d'afficher le message brutale.

Utiliser des fonctions pour tout c'est bien aussi pour peux que ton code au final soit structurer et qu'une fonction ne fait qu'une chose et qu'elle le dit (par sont nom, par exemple echo n'envois pas de sms ;))

pour les requêtes préparée il y a un exemple il y a peu de chance que tu ai un problème en insertion ou sur un prédicat.

mais il est préférable (pour moi d'utiliser les bind (bind_by_name etc).

Pour la dernière chose, en clair ton code est pas mal et un code est toujours améliorable le code parfait c'est une utopie ;)
Je t'assure que certain 'pro' ne font pas du code comme toi (malheureusement).

@+
Il en faut peu pour être heureux ......

Eléphant du PHP | 386 Messages

02 oct. 2014, 23:37

Merci pour tes messages encourageant, je vais faire les corrections nécessaires que tu m'as proposé.
Donc je vais rester sur du procédural, mais avec un codage bien conventionné :wink:

Le projet que je veux faire, se ferais plus facilement avec la POO, mais j'essaye de rendre mon code structuré avec des fonctions, pour me repérer.
C'est un petit réseau social sur le thème de l'astronomie avec :
- messagerie privée
- t'chat
- système d'amis
- un jeu de rôle sur le monde spatial avec canvas 3D (webGL) qui sera long à créer, avec les cours sur canvas et les librairies ça devrait le faire)
- album photos
etc...

C'est long de relier des membres entre eux et de créer un environnement social tout en PHP / Mysql, y'a beaucoup de choses à faire, mais petit à petit, avec des bonnes astuces comme les tiennes, j'arriverais à avoir quelque chose de semi-professionnel O:)

Avatar du membre
Modérateur PHPfrance
Modérateur PHPfrance | 8758 Messages

03 oct. 2014, 20:05

si c'est tous l'art du retour ;)

alors pour faire simple. ce que j'indiquais (mais mal) c'est qu'il est plus intéressant de séparer ce que l'on appel le modèle (l'accès aux données en général) de la vue (affichage).
Du coup tu me vois venir avec mes gros sabots, y ale modèle, la vue => et merde il parle de mvc :)


le truc c'est qu'avec ton code si tu change un truc dans la base ça impact l'affichage ou autre et au final l'utilisation de la base et de l'affichage ce mélange.

tu peux très bien avoir
<?php

// récupère la liste des articles

function getListArticle() {
    $sql = 'SELECT colo,nne FROM article';
// etc avec PDO tu peux faire un fetchAll pour aller plus vite, ilte retourne un tableau
//closecursor()
    return $datas; // datas c'est un tableau qui contient toutes les lignes retournées par la requête. 
}


// affiche unes liste d'article
function afficheListeArticle($liste) {
    if (empty($liste) || !is_array($liste)) {
// bon ben la boulette c'est pas exploitable on affiche un message genre pas d'article
        echo 'Désolé il n\'y a pas encode d\'article disponible';
    } else {
        foreach ($liste as $article) {
            echo 'titre : ', $article['titre'], ' - ', $article['date']; // etc
        }
    }
}

?>
Ça tu sépare les deux si tu veux modifier l'un ou l'autre t'es certain ne pas casser celui que tu ne touche pas (enfin si il y a les index des tableaux ^^).

dans l'absolus tu devrais pouvoir faire
<?php
afficheListeArticle(getListArticle());
?>
et si j'étais pointilleux je te dirais que dans ta fonction tu ne met pas de echo mais tu construis une chaîne de caractère en ajoutant les infos dedans et tu retourne cette chaîne ce qui te permet ensuite d'avoir un contrôle j'affiche ou pas (en fonction du sens de la lune si besoin est :mrgreen: )
ce qui ferais => echo afficheListeArticle(getListArticle());

@+
Il en faut peu pour être heureux ......