2010-07-23 4 views
1

D'accord, je travaille sur la création d'un cours membre pour mon site web. Je veux le faire aussi optimisé que possible. Actuellement, le constructeur peut prendre ($ resource) soit un int (pour récupérer des informations d'un membre de la base, basé sur id) ou un tableau de ints (pour récupérer plusieurs membres de la base de données et les stocker dans une variable membre)). Je voudrais savoir s'il y a des améliorations que je peux faire avec mon bloc de code ci-dessous, avant que je passe à la création de plus de parties de mon site Web. Qu'est-ce qui pourrait être changé pour le rendre meilleur? Y a-t-il une meilleure disposition que je devrais suivre pour faire ce genre de chose?Amélioration d'un constructeur

public function __construct($resource) { 
    global $database; 
    if (is_string($resource) || is_int($resource)) { 
      $resource = (int)$resource; 
    $query = $database->query("SELECT * FROM members WHERE member_id = {$resource} LIMIT 1"); 
    $row = $database->get_row($query); 

     foreach ($row as $key => $value) { 
       $this->field[$key] = $value; 
      } 
    } else if (is_array($resource)) { 
    $query = $database->query("SELECT * FROM members WHERE member_id IN(" . implode(",",$resource) . ")"); 
    while ($member = $database->get_row($query)) { 
    $this->member_list[$member['member_id']] = $member; 
    } 
    } 
} 
+4

Vous pouvez commencer par indentation correcte pour un, cela est difficile à suivre ... – deceze

+0

Je ne supporte pas le code mal formaté. Procurez-vous du [style d'accolade approprié] (http://en.wikipedia.org/wiki/Indent_style#Allman_style_.28bsd_in_Emacs.29). –

+0

commentaires @format, l'éditeur SO n'aide pas. –

Répondre

5

Quelques réflexions:

  • Globals sont mauvais
  • appels de base de données doivent être isolés à une couche d'accès aux données, idéalement de la variété ORM afin que vous n'êtes pas en train d'écrire SQL manuel.
  • Et si $resource est "ddd", voulez-vous le membre avec un member_id de 1?
  • Vous ne protégez pas contre l'injection SQL.
  • Votre member_list ne devrait-il pas créer de nouveaux objets Member (ou quel que soit le nom de cette classe) plutôt que d'ajouter simplement les données de ligne?
+0

Je voulais réduire la quantité de requêtes que j'exécuterais, donc pourquoi member_list est un tableau des lignes de la requête MySQL – Chiggins

+0

@Chiggins c'est exactement pourquoi votre constructeur devrait simplement prendre des données de ligne et ne pas faire lui-même une requête . –

+0

Donc, au lieu d'exécuter la requête MySQL dans le constructeur, faites le MySQL ailleurs, puis passez la ligne au constructeur? – Chiggins

5

Tout d'abord, ne travaillez pas dans un constructeur. Deuxièmement, il existe de nombreux forfaits qui font exactement ce que vous essayez de faire, et le font TRÈS bien. Ce que vous essayez d'écrire s'appelle un modèle. C'est une classe qui reflète une table dans la base de données. Utilisez un package ORM (Object Relational Mapper) mature tel que Propel ou Doctrine pour générer automatiquement des classes basées sur des schémas de base de données. Personnellement, je recommande Propel sur Doctrine (même si ce sont deux excellents forfaits).

Aussi, je vous recommande d'utiliser le symfony php framework, qui intègre Propel comme ORM (encore une fois, vous pouvez utiliser Doctrine comme un ORM alternatif avec Symfony).

Enfin, n'utilisez pas de globaux. Ecrire un cours autour de n'importe quelle ressource qui accède. Faire un appel statique pour récupérer son instance singleton comme Database :: getInstance() est un moyen très préféré d'accéder à la base de données (ou à toute autre ressource).

Bonne chance

+0

+1 pour ne pas travailler dans un constructeur. http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ – Gordon