Mon code est-il bien codé ?

Répondre


Cette question est un moyen d’empêcher des soumissions automatisées de formulaires par des robots.
Smileys
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :!: :?: :idea: :arrow: :| :mrgreen: =D> #-o =P~ :^o :non: :priere: 8-|
Voir plus de smileys
  Revue du sujet
 

  Étendre la vue Revue du sujet : Mon code est-il bien codé ?

Re: Mon code est-il bien codé ?

par moogli » 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());

@+

Re: Mon code est-il bien codé ?

par nico44530 » 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:)

Re: Mon code est-il bien codé ?

par moogli » 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).

@+

Re: Mon code est-il bien codé ?

par nico44530 » 02 oct. 2014, 12:59

Dans mon cas, quelles sont mes fonctions qui ne sont pas bien ?

Re: Mon code est-il bien codé ?

par sirakawa » 02 oct. 2014, 09:02

Juste sur les fonctions:
Ensuite, c'est vrai que j'utilise des fonctions pour presque tout
Il n'y a rien d'anormal, au contraire, à utiliser des fonctions: le code principal en est clarifié; du genre au lieu d'avoir trente lignes de code dans un case, y avoir juste un appel de fonctoin.
La remarque de mooli est que une fonction ne devrait que renvoyer un résultat (Sirakawa : sous forme de tableau si besoin, sérialisé peut-être) et ne rien afficher, une fois qu'elle est au point (des affichages de contrôle en cours de mise au point sont souvent nécessaires)

Re: Mon code est-il bien codé ?

par nico44530 » 02 oct. 2014, 03:19

Salut moogli,

Je vais essayer de répondre à tous tes messages :
Exemple ta fonction login prend deux paramètres que tu écrase directement avec $_POST['email'] et $_POST['mdp']
tu vire ça et utilise la fonction directement ainsi login($_POST['email'], $_POST['mdp']);
le principe est le même pour session.
Tu veux dire que je peux faire ça dans la page du formulaire : login($email, $mdp); ?

Pour le problème de ma classe Bdd, la voici :
class Bdd {
	public static function connect()
	{
		try
		{
			$connection = new PDO(DNS, USER, PASS, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'));
			$connection -> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
		}
		catch (Exception $e)
		{
			die('Échec lors de la connexion : ' . $e->getMessage());
		}
		return $connection;
	}
}
J'ai pas trop compris ce que je dois faire pour une meilleure pratique de cette classe.

Ensuite, c'est vrai que j'utilise des fonctions pour presque tout :?
Mais ça fait quoi d'utiliser des fonctions qui servent à faire des requêtes et à afficher le contenu de la BDD ?
Pour ce qui est de requête préparée pourquoi pas (certain langage ne propose pas d'alternative). Ceci dit sachant qu'elle est mise en cache que sur la connexion que par défaut elle est émulée et que tu utilise le passage de paramètre d’exécuté il n'y pas de réel intérêt hors mis celui de ne pas gérer la "protection" de données où tu va te faire couillonner (désolé) quand tu va vouloir utiliser ce type de requête pour un paramètre dans une limite (ou similaire) et la chose va coller une chaîne de caractères à la place d'un entier (he oui vu que php n'est pas typé mais sql oui, mais s'il y a pas mal d'auto boxing tous ce qui est est chaine de caractère et traité comme tel).
pour info en sql insérer une chaine de caractère dans un entier fonction ne pour peux la chaine contienne un entier la conversion étant réalisée, par contre cela en fonctionnera pas lorsqu'un entier est demandé (dans mon exemple select * from latable where machin='bidulle' limit :start,10 sera remplacé en select * from latable where machin='bidulle' limit '0',10 par exemple et la vautrage assuré).
Là j'ai pas trop compris, tu pourrais citer un exemple ?
Si la variable que je passe en paramètre dans la fonction doit être un entier, je fais une condition avec int() pour pas me faire "couillonner" ?

Pour le code redondant, je pense à la répétition de preg_match pour vérifier certaines choses, je vais me pencher sur filter_var ;)
Ensuite les conventions de codages : Je modifierais mon code :
- Ajouter plus de commentaires sur du code important.
- Coder en anglais (noms de variables etc...).
Au final comme le Nestecha, c'est surtout pinaille & co surtout si tu ne compte pas en faire ton métier
J'ai pas trop compris non plus, tu pense que mon code est tout à refaire ? Que c'est le bordel là-dedans ? 8-|

Pour la POO, je vais quand même m'y remettre, me faire aider par des personnes réelles (pas virtuelles).

Je veux que mon code ressemble à quelque chose à un meilleur niveau que aujourd'hui pour de meilleurs performances d'exécutions.

Dans l'attente de ta réponse
Merci ;)

Re: Mon code est-il bien codé ?

par moogli » 01 oct. 2014, 23:40

salut,

Je vais te parler de bonne pratique autant en php que sql.

=> Tu n'as pas l'air d'utiliser de variable gloable dans les fonctions (or super globales) ce qui mais tu peux faire mieux : ne pas utiliser de globale du tous.

Exemple ta fonction login prend deux paramètres que tu écrase directement avec $_POST['email'] et $_POST['mdp']
tu vire ça et utilise la fonction directement ainsi login($_POST['email'], $_POST['mdp']);
le principe est le même pour session.

Pourquoi ?
Parce que tu va pouvoir obtenir ainsi des fonctions isolable que tu pourras ainsi tester (peux être pas avec phpunit mais tu pourras utiliser la fonction dans un cas passant ou pas et voir ce que ça donne).

Dans le même genre je ne sais pas comment est faite la classe Bdd mais je suppose qu'il y a une sorte de singleton pour ne pas instancier 50 fois pdo vu qu'il s'agit d'une méthode statique.
Ceci n'est pas une bonne pratique il est préférable de passer en paramètre l'objet pdo que tu va fournir aux fonctions dont tu a besoin.
Cela te permet de gérer la chose comme tu souhaite ou créer un wrapper perso que tu pourra changer avec autre chose (il suffit d'utiliser une interface) si tu souhaite (imagine que change ton sgbd par un système de fichier ;) ).

Pour ce qui est des requêtes SQL il y a moyen d'optimiser tes requetes (par exemple utiliser un select dans un insert ça fonctionne).

Dans une logique d'utiliser un motif de conception type MVC les fonctions "utilitaires" ne doivent pas faire d'affichage au pire retourner un message d'erreur ;)

Pour ce qui est de requête préparée pourquoi pas (certain langage ne propose pas d'alternative). Ceci dit sachant qu'elle est mise en cache que sur la connexion que par défaut elle est émulée et que tu utilise le passage de paramètre d’exécuté il n'y pas de réel intérêt hors mis celui de ne pas gérer la "protection" de données où tu va te faire couillonner (désolé) quand tu va vouloir utiliser ce type de requête pour un paramètre dans une limite (ou similaire) et la chose va coller une chaîne de caractères à la place d'un entier (he oui vu que php n'est pas typé mais sql oui, mais s'il y a pas mal d'auto boxing tous ce qui est est chaine de caractère et traité comme tel).
pour info en sql insérer une chaine de caractère dans un entier fonction ne pour peux la chaine contienne un entier la conversion étant réalisée, par contre cela en fonctionnera pas lorsqu'un entier est demandé (dans mon exemple select * from latable where machin='bidulle' limit :start,10 sera remplacé en select * from latable where machin='bidulle' limit '0',10 par exemple et la vautrage assuré).

=> closeCursor() : très bien peu de personne y pense c'est un très bon point ;)

Tu as du code redondant (validation d'email) tu peux créer une fonction pour cela ou utiliser la fonction php filter_var à la place;)

sinon, pour finir sur une bonne note, le code est clair et indenté (bon manque de commentaire ou moins de doc) c'est important est assez rare.

pour ce qui est des conventions de codage je t'invite à regarder ce site http://www.php-fig.org/ (pas mal des articles sont traduit en fr) même si tout (pour moi et pour ce qui est des psr-1 et 2) n'est pas parfait c'est une bonne base.

Au final comme le Nestecha, c'est surtout pinaille & co surtout si tu ne compte pas en faire ton métier ;)

si tu veux aller plus loin regarde du coté des motifs de conception (design pattern), la base étant le mcv (le singleton annoncé tout a l'heure en est un autre) et bien sur la poo pour une conception de plus au niveau.

Par contre n'utilise pas la poo a contre coeur ce n'est pas de la peine de le faire "pour faire de poo".
C'est incontournable pour un pro mais sinon ce n'est pas un problème ;)

@+

Re: Mon code est-il bien codé ?

par nico44530 » 01 oct. 2014, 11:05

Problème résolu
Pour les balises HTML, j'utilise htmlspecialchars qui était mal placé dans mon code.
Et pour éviter d'enregistrer des caractères spéciaux, je fais un preg_match pour chaque champs à l'inscription.
(Ça fait beaucoup d' if)
if(!preg_match("/^[a-z]+$/", $nom)){
	echo '<span class="error">Les caractères spéciaux ne sont pas autorisés</span>';
} elseif(!preg_match("/^[a-z]+$/", $prenom)){
	echo '<span class="error">Les caractères spéciaux ne sont pas autorisés</span>';
} elseif(!preg_match("/^[a-z]+$/", $pseudo)){
	echo '<span class="error">Les caractères spéciaux ne sont pas autorisés</span>';
} else {
// Ma requête
}
Pour les fonctions createFeed() et displayFeed() ainsi que les autres, j'ai bien modifié l'emplacement de htmlspecialchars à l'affichage et ça fonctionne, les balises s'enregistrent mais ne modifient pas mon code, elle s'affichent juste :wink:

Re: Mon code est-il bien codé ?

par xTG » 30 sept. 2014, 20:39

Pour le htmlspecialchars, si je mets pas cette fonction aux variables d'un enregistrement dans une base, des personnes pourrons stockés du code html dans ma base ?
En effet. Sauf que du code HTML non exécuté ne sert à rien. Donc si c'est juste stocker... ;)
Ton but est-il d'interdire les balises HTML dans différents champs ?
Si tel est le cas utilises plutôt strip_tags pour les supprimer.

Re: Mon code est-il bien codé ?

par nico44530 » 30 sept. 2014, 20:00

J'ai mis un commentaire à chaque début de fonctions pour indiquer ce quelle fait.
Et mettre un commentaire aux lignes importantes de la fonction n'est pas nécessaire (je suis pas à l'école)
Je sais ce que font chaque lignes de chaque fonctions

Merci pour la variable $defaut qui n'est pas à la bonne place ;)

Re: Mon code est-il bien codé ?

par sirakawa » 30 sept. 2014, 14:17

Eh bien defaulkt devrait être défini dans le else qui est le seul endroit où il est utilisé.
Pour les commentaires, l'expérience montre que l'auteur lui-même a du mal à s'y retrouver sans commentaires; à plus forte raison, s'il prétend que soncode serve d'exemple à d'autres.
Je connais une filière informatique où l'absence de commentaires ou d'indentation entraîne une note au-dessous de la moyenne, même si le code fonctionne.
Enfin, il suffisait de commenter la fonction:
// requiert une connaissance de PDO, avecun mélange de OOO et de procédural...

Re: Mon code est-il bien codé ?

par nico44530 » 30 sept. 2014, 13:29

//ce genre de concaténation me laisse perplexe
$avatar = $_SERVER['DOCUMENT_ROOT'].'/users/upload/avatars/'.$donnees['id'].'-mini.jpg';
//mal placé puisque ne servant que dans un cas
$defaut = $donnees['sexe'] == 'homme' ? 'homme.jpg' : 'femme.jpg';
$avatar est utilisé pour tester si le fichier existe, sinon on affiche $defaut.
Ils sont placés dans la fonction, pour éviter de les réécrire à chaque fois dans un code.

Re: Mon code est-il bien codé ?

par nico44530 » 30 sept. 2014, 13:26

Sirakawa, si tu connais un minimum le PDO, tu saurais que Bdd::connect, c'est la classe de connexion à la base de données.
Ensuite, je récupère l'id du membre connecté dans la fonction avatar, pour afficher l'avatar qui est renommé : $id.png

Pour l'absence de commentaires, je suis seul à travailler sur ce site, pour l'instant je n'ai pas besoin d'écrire de commentaires
(Si tu veux des détails, je peux t'en donner :wink: ).
Ayant crée mes fonctions, je connais les descriptions et les paramètres.

$req -> closeCursor(); Ferme le curseur, permettant à la requête d'être de nouveau exécutée (https://php.net/manual/fr/index.php)

Re: Mon code est-il bien codé ?

par sirakawa » 30 sept. 2014, 13:00

Absence de commentaires UTILES
Pas de description précise des fonctions
Pas d'explications sur les paramètres
ex
/ Affichage de l'avatar du membre connecté
//$id c'est quoi ?
//Bdd::connect caq sort d'où?

function avatar($id){
$req = Bdd::connect() -> prepare('SELECT id, sexe FROM users u INNER JOIN users_infos ui ON u.id = ui.userid WHERE u.id = :id');
$req -> execute(array('id' => $id));
$donnees = $req -> fetch();
//ce genre de concaténation me laisse perplexe
$avatar = $_SERVER['DOCUMENT_ROOT'].'/users/upload/avatars/'.$donnees['id'].'-mini.jpg';
//mal placé puisque ne servant que dans un cas
$defaut = $donnees['sexe'] == 'homme' ? 'homme.jpg' : 'femme.jpg';

if(file_exists($avatar)){
echo $donnees['id'].'-mini.jpg';
} else {
echo $defaut;
}
$req -> closeCursor(); //que fait cette fonction?
}

Re: Mon code est-il bien codé ?

par nico44530 » 30 sept. 2014, 12:57

Merci pour vos messages :

POO, j'ai essayé de suivre le cours sur openclassroom, et même en lisant le cours plusieurs fois et en pratiquant, j'ai toujours rien compris #-o
La méthode procédurale me va bien, même si je me prend la tête avec beaucoup de codes, je me comprend dans mon code :)

Pour le htmlspecialchars, si je mets pas cette fonction aux variables d'un enregistrement dans une base, des personnes pourrons stockés du code html dans ma base ?
Pour l'anti-injection SQL, j'utilise les requêtes préparées ;)