Critique de mon Premier Vrai Script de POO

[Eluox]

Bonjour, je me met a la POO, intensivement, et je voudrais votre avis sur ma première class.

Merci a Sephi pour les petits coups de main
Ne pas faire gaffe au erreurs dans les commentaires en anglais, c'était fait vite fait

Et pour info, MySQL c'est mysqli

Code PHP :

<?php

/**

 * Player

 * 

 * @author  El[u]ox <Lakers_suporter@hotmail.com>

 * @copyright 2008

 * @version 1.0

 * @package Player

 */

class Player implements IPlayer

{

    //Private Player Information

    private $playerId;

    private $name;

    private $mail;

    private $password;

    private $avatar;

    private $signature; 

    

    public function create($newPlayerInfo)

    {

        return System::addPlayer($newPlayerInfo);

    }

    

    public function load($playerID)

    {

            //Try to connect mysql

            $mysql = Database::GetInstance();

            

            //Select User Information

            $sql = sprintf(

                           "SELECT email, name, password, avatar, signature

                            FROM player

                            WHERE id = %d;",

                           (int) $playerID

                            );

                //Do the Query               

                $aPlayer = $mysql->sqlFetch($sql,MYSQLI_ASSOC);

                

                $this->playerId = $playerID;

                $this->mail = $aPlayer[0]['email']; //And assigning user information

                $this->name = $aPlayer[0]['name'];

                $this->password = $aPlayer[0]['password'];

                $this->avatar = $aPlayer[0]['avatar'];

                $this->signature = $aPlayer[0]['signature'];

    }

    

    public function getInfo($sKey) { //Recuperate an information 

        return $this->$sKey;

    }

    

    public function changeAvatar($newURL) //Change Avatar

    {      

    

           $Extension = strtolower(substr(strrchr($newURL,'.'),1));

           $allowedExtension = array('jpg','bmp','gif','png');

           

           if(in_array($Extension,$allowedExtension) == false){

                return 'Synthaxe URL incorrect';

                exit;

           }

           

           $mysql =  Database::GetInstance(); //Try to connect Mysql

           

           $sql = sprintf("UPDATE player SET `avatar` = '%s' WHERE id = %d",strip_tags($newURL), $this->playerId); //Update query

           $mysql->sqlQuery($sql); //Do the query

           

           return 'Avatar Changé avec succès';

    }

    

    public function changeSignature($newSign) //Change Signature

    {

           $Extension = strtolower(substr(strrchr($newSign,'.'),1)) ;

           $allowedExtension = array('jpg','bmp','gif','png');

           

           if(in_array($Extension,$allowedExtension) == false){

               return 'Synthaxe URL incorrect';

               exit;

           }

           

           $mysql =  Database::GetInstance(); //Try to connect Mysql

           

           $sql = sprintf("UPDATE player SET `signature` = '%s' WHERE id = %d",strip_tags($newSign), $this->playerId); //Update query

           $mysql->sqlQuery($sql); //Do the query

           

           return 'Signature Changé avec succès';

    }

    

    /**

     * @return string $this Dumping of $this

     */

    public function toString() {

        echo '<pre>', var_dump($this), '</pre>';

    }

}

?>

[Sephi-Chan]

Salut,

Grosse remarque, pourquoi utiliser un tableau contenant les informations ? Rien ne le justifie du point de vue de la conception. Les attributs de ton objets (le nom, l'identifiant, l'email, etc.) doivent être des attributs.

Si c'est parce que tu as la flemme de faire des setName, getName, etc. tu peux implémenter les méthodes magiques __get() et __set(). Je te laisse te renseigner sur leur utilisation. Tu n'es pas obligé de détourner la nature d'un objet pour subvenir à tes besoins techniques. Le mieux reste cependant d'écrire tes setters et tes getters à la main.


Sephi-Chan

[Ren Nelos]

Bonjour,

Ce qui suit sont des remarques personnelles très influencées par des pratiques personnelles =]

Je commencerait par le début en disant, ça non :

Code PHP :

<?php 

//Private Player Information

private $player = array( 

                        'name' => '', 

                        'mail' => '',

                        'password' => '',

                        'avatar' => '',

                        'signature' => ''

                         ); 

[Eluox]

Merci a vous.

J'ai modifier te éditer mon premier message.
Je suis toujours la pour plus de conseils.

Cordialement,

[Ren Nelos]

J'aurais une certaine tendance à dire qu'il faudrait un pattern singleton pour cette classe, non ?

Code PHP :

<?php 

class MaClasse {

   private static $instance;

  

   private function __construct() {

     }

  

   public static function getInstance {

     if ( !isset(self::instance) ) {

       self::$instance = new MaClasse();

       }

     return self::$instance;

     }

} 

[naholyr]

Je n'aurais pas mis la requête dans le constructeur, car sinon comment veux-tu créer un nouveau joueur, qui n'existe pas encore dans la base (par exemple à l'inscription) ?

J'aurais plutôt fait un vrai début de Data Object avec :
- les getters (très utile pour faire des traitements avant de renvoyer une valeur)
- les setters (très utile pour faire les vérification avant d'affecter la valeur)
- les loaders (va charger les infos depuis la bdd)
- le saver (sauve l'instance en cours dans la bdd)
ces deux derniers éléments imposant de stocker l'ID dans les attributs de l'objet

Typiquement, l'utilisation donnerait ce genre de codes :

Code PHP :

<?php 

// Inscription

$player = new Player;

$player->setName($login);

$player->setMail($email);

$player->setPassword($password);

try {

  $player->save();

} catch (Exception $e) {

  die('Impossible de créer le compte, erreur : ' . $e->getMessage());

} 

[Sephi-Chan]

Je suis d'accord pour l'utilisation d'une méthode load(), je lui ai même précisément fait l'exemple de l'inscription ou la connexion d'un joueur.

[Eluox]

Un grand merci a vous, je boss sur ça ce week end, et je vous montre le script

Par contre, pour la création d'user, (setName, ect et save pour toi script), pourquoi ne pourrais-je pas faire create($name,$pass), ect


Cordialement, et merci

[naholyr]

Ce serait simplement moins homogène Enfin c'est mon avis, ce genre de chose est assez subjectif.

De toute façon quand tu conçois une API il faut réfléchir en terme de besoins uniquement.
Quand tu écris une page web, tu pars du HTML que tu veux générer : tu as ton design, à partir duquel tu fais ta page HTML+CSS, à partir duquel tu écris ton code PHP qui se chargera de générer cet HTML. Tu pars du besoin, et tu y réponds.
Là c'est pareil : pars donc du code que tu aimerais avoir lorsque tu *utilises* ta classe Player, dans les différents cas d'utilisation, comme je l'ai fait plus haut. Ne pense pas à ce moment là à comment doit être faite la classe Player, juste à comment tu aimerais l'utiliser et après tu réponds à ce besoin dans ta classe, tout simplement.

Tu verras que refléchir dans ce sens permet de simplifier la conception.


P.S: ou comment faire de la programmation pilotée par les tests sans le savoir

[Eluox]

Merci beaucoup naholyr,
tu m'épatera toujours