Discussion :

[Tsilefy]

Ah oui, et

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

echo $utilisateur->__get('utilisateurActif');

La méthode magique __get ne s'utilise pas comme ça.

Et de toutes façons, mieux vaut utiliser les méthodes magiques le moins possible. Remember: tout devrait être explicite, clair, facile à comprendre.

[Kontas]

Tes classes sont à l'évidence non terminées (il y a encore plein d'erreurs, d'utilisation de variables inexistantes ou de propriétés inaccessibles, etc...), donc j'en tiens compte.

Je rappelle que ce thread porte sur "les bonnes pratiques en POO", pas sûr "est-ce que ça marche?".

Nous sommes d'accord sur ce point. Il est fortement possible qu'il y'ai des erreurs dans ce genre, mais ce qui m'interesse c'est justement tous les points sur les bonnes pratiques que tu as soulevé plus bas, et je t'en remercie

- Choisis un style et garde-le. Dans un classe, tu préfixes une propriété privée par _ (je ne suis pas fan, ça vient des langages qui n'ont pas de notion de visibilité. Le mot-clé private est suffisamment clair en PHP. Après c'est juste mon goût personnel), et dans l'autre tu ne le fais pas.

Il semble y avoir plusieurs école à ce niveau la, mais je suis ouvert à toute les écoles, si l'idée des préfixes et d'indiqué la visibilité, c'est sur que du moment on l'on indique public,protected, private cela ne sert plus à rien.

- Les noms de classes en majuscule (StudlyCaps), les noms de méthodes commencent par une minuscule, mais chaque mot commence ensuite par une majuscule (camelCase). Les variables aux choix ($camelCase ou $under_score).

Oky, donc j'opte pour un schéma qui reste assez proche de mes habitudes

classe : MaClasse
Méthode : maMethode
Variable : maVariable

- Spécifie clairement la visibilité de tes méthodes par private/protected/public. En POO (et en programmation en général), beaucoup de chose repose sur la clarté d'intention. Quand tu as l'occasion d'exprimer cette intention (méthode publique ou pas), fais-le. Pense au fait que dans 3 mois ou dans 6 mois tu reliras ton code (ou un autre développeur le lira) et fais en sorte que ton code soit facile à lire et à comprendre. Un code facile à comprendre = un code facile à modifier, à corriger et à étendre.

C'est certain que ca permet de connaitre l'intention du développeur car même si public est attribué par défaut, cela aurait très bien pu être un oubli, et j'imagine que cette clarté doit être également appliqué aux variables de la classe.

Une classe ne dois pas avoir de code directement exécutable, c'est-à-dire affectant directement le contexte supérieur. Ce n'est pas de la responsabilité de ta classe d'arrêter le script et d'afficher un message HTML. Tu dois relancer PDOException, lancer tes propres exceptions pour $messages, ou retourner false (indiquant un échec), etc... mais tu ne dois jamais briser l'encapsulation (qui est la base même de l'OOP). C'est le script qui utilise ta classe qui doit décider quoi faire en cas d'erreur. Imagine que tu décides plus tard qu'un cas d'erreur, tu veux aussi envoyer un email au développeur, ou faire un roll back sur une transaction SQL. Est-ce que tu vas modifier une à une toutes tes méthodes pour ça? C'est une très -mauvaise solution. La meilleur solution est de transférer ce comportement à la classe/au script qui utilise ta classe: envoie un message signalant une erreur, et laisse la classe appelante décider quoi faire.

Effectivement c'est bien plus logique, Je peut par exemple revoyer TRUE, ou la valeur si c'est OK, et renvoyer FALSE si ca ne l'ai pas, par contre dans ce cas là, comment récupérer l'erreur PDO, elle sera disponible dans l'objet que je transfère à ma classe $pdo, ou dans l'instance $prep, car dans le cas ou c'est dans l'instance $prep, une fois le return de ma méthode executé, elle n'existera plus ?!

- Tu remarques que tu répétes plusieurs fois la même séquence:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
$query = "...";
$prep  	= $this->_pdo->prepare($query);
$prep->bindValue(...);
$prep->execute();
$data 		= $prep->fetch();
//etc...

Il faut refactoriser tout ça dans une méthode privée ou protégée afin d'éviter toutes ces répétitions.

Par exemple en ajoutant une méhode à ma classe PDO, qui accepterais $query et qui renverra $data ?! Il faut juste que je trouve comment factoriser la partie Bind.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
//Clore la requête préparée
        $prep->closeCursor();
        unset($prep);

Ces deux lignes sont inutiles. On peut déjà contester l'utilité de closeCursor() pour mysql, surtout dans un script PHP, mais en plus dans ton cas la variable $prep devient hors de portée dès que la méthode est terminée (après "return"), et donc PHP l'efface automatiquement et libère les ressources qui y sont associées.

Oky, merci pour l'info

- Je ne suis pas fan de ta méthode "magique" hydrate (encore une fois, question de clarté). Je préférerais que utilisateurSQL crée directement l'objet utilisateur, avec toutes les bonnes propriétés, en utilisant les setters si nécessaire. Si je parcours le code rapidement, "hydrate" est une boîte noire: je ne sais pas ce qu'elle fait, il faut que je regarde le code source pour cela (et encore, la source n'est pas facile à comprendre: il fait une concaténation de string, puis appelle une méthode dynamiquement. Encore une fois, oui, ça marche (pour l'instant), mais c'est fragile. Que se passe-t-il si tu as des méthodes autres que des set* ? Tu vas modifier hydrate() à chaque nouveau format de méthode?

Je vais voir pour remplacer ça, c'est effectivement une technique de flemmard , chose assez étonnante, en lui passant en argument le nom d'une variable qui n'existe pas, elle la crée dans l'objet.

- Fais gaffe à ctype_alnum. Suivant le locale présent sur le serveur et déclaré, ce genre de test peut donner false:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

var_dump(ctype_alnum('accentué'));

Hum, je comprend mieux pourquoi mes valeurs accentuées ne fonctionnaient pas ! Merci

- Mets une seule classe par fichier.

Ca semble assez logique ^^

Le ?> final est inutile lorsque ton fichier ne contient que du code PHP. Au contraire, il peut être nuisible dans certains cas, si tu mets des lignes vides après (ou si ton éditeur en rajoute dans ton dos!).

J'avais déjà lu cette recommandation, mais par contre, pour un script php quelconque ou l'ont trouvera du html par des echo, doit on également enlever le ?> ?

Autrement, tu as bien compris le concept de Data Mapper, qui sépare la classe modèle de la classe persistance. Le principal intérêt c'est que ça te permet de réutiliser ta classe modèle ailleurs (par exemple dans la validation de formulaires, l'envoi d'emails, dans un API REST, dans un json pour afficher des tableaux etc...)

Je n'irais pas jusqu'à dire que j'ai compris, mais plutôt que je commence à découvrir / comprendre ces notions.

Merci encore pour ces précieux conseils !!

[Kontas]

Ah oui, et

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

echo $utilisateur->__get('utilisateurActif');

La méthode magique __get ne s'utilise pas comme ça.

Et de toutes façons, mieux vaut utiliser les méthodes magiques le moins possible. Remember: tout devrait être explicite, clair, facile à comprendre.

Dans ce cas, je vais reprendre également cette partie.

[Tsilefy]

par contre dans ce cas là, comment récupérer l'erreur PDO, elle sera disponible dans l'objet que je transfère à ma classe $pdo, ou dans l'instance $prep, car dans le cas ou c'est dans l'instance $prep, une fois le return de ma méthode executé, elle n'existera plus ?!

C'est pour ça qu'il est préférable de lancer toi-même une exception plutôt que false.

Il faut juste que je trouve comment factoriser la partie Bind.

Certains IDE comme PHPStorm peuvent le faire pour toi. Tu pourais, par exemple, refactoriser bind en injectant dans ta nouvelle méthode les paramètres de bind sous forme de tableau d'options. Ou utiliser un tableau d'options pour passer tous les paramètres, etc...

J'avais déjà lu cette recommandation, mais par contre, pour un script php quelconque ou l'ont trouvera du html par des echo, doit on également enlever le ?> ?

Quelle que soit la page, avec ou sans html, S'il n'y a plus rien après le ?> final, mieux vaut enlever le ?>

[Kontas]

Avant d'aller plus loin sur les élements de reponse, je me permet de revenir sur l'affranchissement de la fonction Hydrate() qui permet d'attribuer les valeurs aux variables

Si je veux pouvoir me débarrasser de cette fonction, il faut que je fasse ceci dans le contructeur :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
 
 
function __construct($donnees){
 
     $this->idutilisateur = $donnees['idutilisateur'];
     $this->utilisateurNom = $donnees['utilisateurNom'];
}

Si je crée un nouvel objet :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
$donnees = ['utilisateurNom' => 'nom'];
$utilisateur = new utilisateur($donnees);

je vais forcement avoir une erreur car je n'ai pas passé l'id de l'utilisateur dans le tableau $donnees. Mais j'ai besoin de pouvoir attribuer un id via le constructeur par exemple en sortie de base de donnée. Donc il faut que je vérifie l'existence de la clé dans le constructeur dans le tableau $donnée par exemple comme ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
 
if(array_key_exists('idutilisateur', $donnees)){
       $this->idutilisateur = $donnees['idutilisateur'];
}

Et forcement, il est nécessaire de répéter l'opération autant de fois que j'ai de variables dans mon objet. Mais je trouve ce bout de code assez contraignant, et j'ai bien l'impression que si je le simplifie pour éviter de répéter à chaque fois, je vais me retrouver avec un truc assez proche de la fonction hydrate que j'essaie d'éviter.

[Tsilefy]

Je t'ai juste donné des indications, pas des règles :-)

Mon principal problème avec hydrate c'est la concaténation de string, parce que c'est magique (çad implicite) et que ça ne permets pas une compréhension immédiate du code (bien évidemment, dans cet exemple la portée est assez limitée pour que ça ne soit pas très grave, mais dans un code plus important ça peut gêner. D'autres te diraient aussi de ne pas faire de concaténation dans une boucle, mais ce n'est pas important pour le moment.

Après c'est à toi de voir ce qui convient dans ton cas.

Dans mes classes, je ne donne pas à mes objets métiers (domain objects) la responsabilité de se construire eux-mêmes, je n'ai donc pas de méthode de type hydrate. D'abord parce que le mots ont un sens, et hydrate veut dire transformer des données venant d'une source de données persistante (généralement une base de données) en objets PHP. Un objet métier ne doit donc pas avoir de méthode hydrate, un objet Mapper, oui, parce que ça correspond à la définition même de Mapper.

Ensuite, même si tu remplaces hydrate() par build() pour utiliser un autre verbe, pour moi ça ne fait toujours pas partie de la responsabilité de l'objet. Je préfère utiliser directement les paramètres de l'objet, dans un formulaire par exemple, ce qui me permet de lui donner uniquement les informations nécessaires à ce stade (pour un nouvel enregistrement, un ID n'est pas nécessaire par exemple).

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
$user = new User;
$user->setName($name);
$user->setEmail($email);
//etc... Avec un IDE, ça ne prends pas de temps.

Et si je me retrouve à réutiliser le même formulaire plusieurs fois, et donc à répéter le même code, je crée une classe formulaire et dans ce cas, oui, je refactorise le tout dans une méthode. Mais une méthode qui appartient au formulaire, et sans concaténation de string.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
$user_form = new UserForm();
$user_form->createUser(null, $name, $email,...); //cette méthode utilise les set* en interne
 
//ou pour un edit
$user_form->updateUser($id, $name, $email,...); //pareil

Eviter les répétitions, c'est bien. Mais ça ne doit pas être au détriment de la sémantique (le sens) des objets. Une classe UserMapper (ou UtilisateurSQL) construit des objets User en les hydratant; Une classe UserForm construit des objets User à partir des valeurs d'une requête POST. Un objet User ne peut pas construire des Objets User, il se contente de réprésenter tes données métiers.

Bien évidemment, dans un exemple limité comme celui que tu as donné, tout cela est un excès d'architecture, du coupage de cheveux en quatre. Mais sur un projet de moyenne envergure, de telles séparations sont nécessaires pour éviter que ton code ne s'enchevêtre dans un spaghetti monstrueux.

Et autre chose que tu devrais regarder, même si ça n'a qu'un rapport lointain avec ton exemple, si tu es en quête des bonnes pratiques: l'utilisation des interfaces. J'irais jusqu'à dire que tu ne fais pas de la POO si tu n'utilises pas d'interfaces (interface au sens large, pas uniquement le mot-clé Interface de PHP). Tu fais juste du procédural caché dans des classes et des méthodes.

[Kontas]

Merci encore pour ces précisions.

Puisque nous sommes dans l'attribution des valeurs aux variables d'une classe, ne serai t'il pas judicieux, de placer dans la méthode Set d'une variable, le contrôle du type de donnée, INT, string, et pourquoi pas un html entities, car habituellement je fais ce genre de contrôle au moment de l'insertion SQL, mais cela ne serai pas plus logique de contrôler ces donnée avant de les exploiter ?!

Je pense déposer un projet GutHub, visant à toujours tirer vers une programmation propre et qui pourra toujours servir de base pour de futur projet.

[tdutrion]

Ah oui, et

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

echo $utilisateur->__get('utilisateurActif');

La méthode magique __get ne s'utilise pas comme ça.

Et de toutes façons, mieux vaut utiliser les méthodes magiques le moins possible. Remember: tout devrait être explicite, clair, facile à comprendre.

Je dirais ici vu qu'on parle de bonnes pratiques de jeter un oeil à PSR (php-fig) qui standardise l'interopérabilité des frameworks et propose notamment PSR-2 pour le code style. Extrait de PSR-2 :

Les noms de propriété NE DEVRAIENT PAS être précédés d'un sous-tiret pour indiquer la visibilité protégée ou privée.

La recommandation PSR n'est pas forcément longue et assez instructive. Elle donne une idée de bonne pratique de style arbitrairement décidé mais utilisé par la plupart des développeurs PHP (contrairement à d'autre coding standards comme ceux de ZF1 qui après avoir suivi psr-2 paraissent des fois pas top...).
Un autre avantage est la possibilité d'utiliser des outils et d'automatiser facilement les vérifications (phpcs --standard=PSR2 ou php-cs-fixer par exemple).

[Tsilefy]

Un bon IDE (comme PHPStorm) te permet de transformer ton code en PSR-compliant en un click :-)

[Tsilefy]

Puisque nous sommes dans l'attribution des valeurs aux variables d'une classe, ne serai t'il pas judicieux, de placer dans la méthode Set d'une variable, le contrôle du type de donnée, INT, string, et pourquoi pas un html entities, car habituellement je fais ce genre de contrôle au moment de l'insertion SQL, mais cela ne serai pas plus logique de contrôler ces donnée avant de les exploiter ?!

- Demande toi déjà si tu as vraiment besoin d'un contrôle de type scalaire (i.e. hors objets et array), et si oui, quel type tu veux contrôler, sachant qu'en PHP la majorité des inputs vient d'une requête, d'un fichier ou d'une base de données, et donc sont des strings. Tu utiliseras donc plutôt is_numeric qu'un int strict, si jamais tu en as besoin.

- Si tu conclus que dans certains cas tu dois vérifier le type (et il y a des cas pour lesquels c'est effectivement nécessaire), oui, vérifie le type dans le setter. Ou attends PHP 7, qui sortira bientôt (dans quelques mois), et qui propose un scalar typehint:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
function setFloat(float $float);

- htmlentities est une fonction de conversion (utilisée comme outil de "sanitization" - nettoyage) liée à un contexte (HTML). Ton objet n'est pas lié à ce contexte, il opère dans une couche complètement différente (la couche métier - business layer). Ton objet peut ensuite être utlisé par un web service, stocké dans une base de données (relationnelle ou non), servi en tant que json, xml, texte, etc.

La "sanitization" doit se faire uniquement dans le contexte d'utilisation: htmlentities dans un template html, json_encode dans une section <script>, etc.