Czy to jest poprawne - w duchu obiektowości?, proszę o komentarze Opcje

[Aztech]

Zacząłem niedawno pisać na poważnie (w końcu praca duplomowa) w php5 i chciałbym się spytać czy to co napisałem, jest w pełni w duchu obiektowości. Może widzicie tam jakieś błędy, może macie jakieś wskazówki. Za wszelkie rady, spostrzeżenia, w szczególności te bardzo krytyczne, będę niezmiernie wdzięczny.

Gwoli wyjaśnienia, klasa ta ma odpowiadać za obsługę użytkownika:
(*) dodawać
(*) usuwać
(*) uaktualniać
(*) parsować poprawność wprowadzonych danych
itp.

A więc jeszcze raz proszę o jak najwięcej uwag i krytyki (ew. pochwały też przyjmuję (IMG:http://forum.php.pl/style_emoticons/default/biggrin.gif) )

[PHP] pobierz, plaintext 
<?php 
 
 
/*
    UWAGA: wszystkie zapytanie sql sformatować odpowiednio dla adodb
*/
 
require_once("../core/adodb/adodb.inc.php");
 
class TUser {
    private $user_id=-1;
    private $nick ="";
    private $name= "";
    private $group_id=0;
    private $password="";
    private $password_crypt="";
    private $email="";
    private $gg="";
    private $photo="";
    private $description="";
    private $phone="";
    private $last_error="";
    private $is_error=0;   //0 - no error, 1 - error
 
################################## need to update all validation functions #####
##############################
 
/********************************************************************************
*****    
                        VALIDATE FUNCTIONS                    
********************************************************************************
*****/
 
    public function validateName($name) {
        // validate if name is correct, name have 2 words which begin with uppercase and s
pace beetween of them
        if (ereg("^[A-z](a-zA-Z/-)*( A-Za-z/-)*",$name)) {
               $this->unsetError();
               return true;           
        }
        else {
            $this->last_error = "NO_VALID_NAME";
            $this->setError();
            return false;
        }       
    }
    
    public function validateEmail() {
        //validate if email is correct
        if (eregi("^([a-z0-9_-.]{1,25})@([a-z0-9_-.]){1,25}$",$this->$email)){
               $this->unsetError();
               return true;           
        }
        else {
            $this->last_error = "NO_VALID_EMAIL";
            $this->setError();
            return false;
        }
    }
    
    public function validatePassword($pass) {
        //6-25 signs, one big letter and one number        
        if (ereg("[A-Za-z0-9]")) {
               $this->setError();
               return true;           
        }
        else {
            $this->last_error = "NO_VALID_PASSWORD";
            $this->setError();
            return false;
        }
    }
    
/********************************************************************************
*****    
                        COMPARE FUNCTIONS                   
********************************************************************************
*****/
 
    public function comparePassword($value="") {
        if ($this->password == $value) {
            $this->unsetError();            
            return true;            
        }
        else {
            $this->last_error = "PASSWORD_NOT_SAME";
            $this->unsetError();
            return false;
        }           
    }    
 
/********************************************************************************
*****    
                        GET FUNCTIONS                    
********************************************************************************
*****/    
    
    public function getUserInfo(){
        return $this;
    }
 
/********************************************************************************
*****    
                        SET FUNCTIONS                    
********************************************************************************
*****/    
 
    public function setError(){
        $this->is_error = 1;
    }
    
    public function unsetError(){
        $this->is_error = 0;
    }
 
    public function setUserID($value="") {
        $this->user_id = $value;
    }
    
    public function setNick($value="") {
        $this->nick = $value;
    }
    
    public function setName($value="") {
           $this->name = $value;
    }
    
    public function setGroupID($value="") {
           $this->group_id = $value;
    }
    
    public function setPassword($value="") {
           $this->password = $value;
    }
    
    public function setEmail($value="") {
           $this->email = $value;
    }
    
    public function setGG($value="") {
           $this->gg = $value;
    }
    
    public function setPhoto($value="") {
        $this->phone = $value;
    }
    
    public function setDescription($value="") {
           $this->description = $value;
    }
    
    public function setPhone($value="") {
           $this->phone = $value;
    }
    
    
    public function setUserFromSQL($table){
        //set user data using result form SQL query
        $this->user_id          =   $table["id_user"];
        $this->nick             =   $table["nick"];
        $this->name             =   $table["name"];
        $this->group_id         =   $table["name"];
        $this->password         =   $table["password"];
        $this->password_crypt   =   md5($table["password"]);
        $this->email            =   $table["email"];
        $this->gg               =   $table["gg"];        
        $this->photo            =   $table["photo"];
        $this->description      =   $table["description"];
        $this->phone            =   $table["phone"];
        $this->last_error       =   "";
        $this->is_error         =   "0";    //no error
    }
       
    public function __construct() {
        $this->user_id          =   "-1";
        $this->nick             =   "";
        $this->name             =   "";
        $this->group_id         =   "0";
        $this->password         =   "";
        $this->password_crypt   =   "";
        $this->email            =   "";
        $this->gg               =   "";        
        $this->photo            =   "";
        $this->description      =   "";
        $this->phone            =   "";
        $this->last_error       =   "";        
        $this->is_error         =   "0";    //no error
    }
    
    public function clearrData() {
        $this->__construct();
    }
 
/********************************************************************************
*****    
                        SET FUNCTIONS                    
********************************************************************************
*****/ 
 
    public function findAllGroupsSQL(&$groups) {
        //fing all groups where belong user
        //if user is set
        if ($this->user_id != "-1") {
            /*
            $sql =   " SELECT user_id, group_id, id_groups, name, moderator_id"
                    ." FROM rbx_group_users LEFT JOIN rbx_groups"
                    ." ON rbx_group_users.group_id = rbx_groups.id_groups"
                    ." WHERE user_id = 3";            
                    */
        }
        else {
            $this->lastError="NO_USER";
            $this->setError();
            return false;
        }
    }
    
    public function userExistsSQL() {
        //connectToADO($db);
        $sql = "select email from rbx_username where email='".$this->email."'";
        //$result = $db->Execute($sql);  
        if (!$result->FieldCount())
            return true;
            else return false;        
    }
    
    public function createUserSQL() {
    //gdy uzytkownik nie istnieje
        if (!$this->userExists()) {
            //connectToADO($db);
            $sql = "insert into 'rbx_user' ( 'id_user' , 'name' , 'pass' , 'email' , 'gg' , 'photo' , 'description' , 'phone' )".
            " values ('', '".$this->name."', '".$this->password_crypt."', '".$this->email."', '".$this->gg."', '".$this->photo."', '".$this->description."', '".$this->phone."')";
            //$result = $db->Execute($sql);          
            $this->is_error = false;      
        }
        else {
            $this->is_error = true;
            $this->last_error = "USER_ALREADY_EXIST";
        }
    }           
 
    public function updateUserSQL() {    
        //connectToADO($db);        
            $sql = "update 'rbx_user'"
            ." set 'name'=".$this->name.","
            ." 'pass'=".$this->password_crypt.","
            ." 'email'=".$this->email.","
            ." 'gg'=".$this->gg.","
            ." 'photo'=".$this->photo.","
            ." 'description'=".$this->description.","
            ." 'phone'=".$this->phone.","
            ." 'pass'=".$this->password_crypt
            ." WHERE 'id_user'=".$this->user_id;            
        //$result = $db->Execute($sql);          
        $this->is_error = false;      
    }
    
    public function deleteCurrentUserSQL() {
        //connectToADO($db);
            $sql = "delete from 'rbx_user'"
            ."WHERE user_id=".$this->user_id;
        //result = $db->Execute($sql);
        $this->is_error = false;
    }
    
    public function deleteUserSQL($db) {
        //connectToADO($db);
            $sql = "delete from 'rbx_user'"
            ."WHERE user_id=".$db->user_id;
        //result = $db->Execute($sql);
        $this->is_error = false;
    }        
    
        
}
 
?>
[PHP] pobierz, plaintext 

Ten post edytował NuLL 28.11.2005, 23:06:30

[krzysztof f.]

Witam Aztech!

Więc zaczynamy.. zajmijmy się duchem obiektowości ; ).
Pierwsza zasada: każdy obiekt w systemie powinien mieć ściśle określoną odpowiedzialność, którą da się opisać jednym zdaniem. Powinien dostarczać implementacji jakiegoś zachowania lub struktury z dziedziny problemu. Jednym słowem musimy wystrzegać się tak zwanych god classes, przeciążonych nadmierną odpowiedzialnością i kodem.

W tej kwestii poradziłeś sobie nie najgorzej, ale też daleko od ideału. Przyjrzyjmy się odpowiedzialności klasy, którą potrzebujesz stworzyć. Będzie to prawdopodobnie klasa opisująca obiekt użytkownika w Twojej aplikacji. Obiekt taki ma identyfikować pojedynczego użytkownika i dostarczać niezbędnych poprawnych danych o nim. Ponadto obiekt taki ma być utrwalany w bazie danych. W tym miejscu powinno nasunąć się już pytanie, czy obiekt użytkownika powinien też odpowiadać za utrwalanie siebie w bazie danych. Zgodnie z architekturą wielowarstwową obiekt użytkownika należy do warstwy logiki biznesowej, a zapisywanie do bazy to już odpowiedzialność warstwy danych. Aby oddzielić zupełnie te dwie odpowiedzialności musielibyśmy się zająć pojęciem ORM, którego niewielki fragment znajdziesz w przykładowym rozdziale z PoEAA http://www.awprofessional.com/articles/article.asp?p=30661.

W twoim przypadku prawdopodobnie wystarczy prostsze rozwiązanie, które w zasadzie już zastosowałeś a nazywa się Active Record http://martinfowler.com/eaaCatalog/activeRecord.html. Jest wzorzec stosowany do tworzenia obiektów dziedziny, których struktura pokrywa się ze strukturą tabeli i które reprezentują pojedynczy rekord takiej tabeli. Oprócz podstawowej niewielkiej ilości logiki biznesowej, potrafią one utrwalać się w bazie danych.

Skoro padła już nazwa jakiegoś wzorca, to możemy być pewni, że część odpowiedzialności prawdopodobnie uda nam się wyłączyć do klasy bazowej. http://www.daholygoat.com/jaws/html/index....ction=Page&id=8

Więc ustaliliśmy już kwestie utrwalania danych http://patternshare.org/default.aspx/Home.MF.ActiveRecord , postanowiliśmy wyodrębnić klasę bazową. Teraz zastanówmy się nad metodą wydobywania danych i konkretyzacji obiektów. Aby odciążyć trochę klasę użytkownika, odpowiedzialność za wyszukiwanie obiektów w bazie należałoby wyodrębnić do oddzielnego obiektu. Przy okazji zastosujemy Separatek Interface http://patternshare.org/default.aspx/Home....aratedInterface ukrywając implementację przed kodem klienta. W praktyce kod klienta będzie wyglądał mniej więcej tak:

[PHP] pobierz, plaintext 
<?php
$oUser = $oUserFinder->findByName( 'Krzysztof' );
$oUser->setNick( 'blazej' );
$oUser->update();
 
//dla kolekcji obiektów
$aUsers = $oUserFinder->findAll();
foreach ( $aUsers as $oUser )
{
   echo $oUser->getName() . '<br />';
}
?>
[PHP] pobierz, plaintext 

Poświęćmy jeszcze chwilę tworzeniu obiektów User. Nie podoba mi się, że pozwalasz tworzyć puste obiekty nie wykorzystując konstruktora. Poza tym konstruktor jest metodą wykorzystywaną tylko w momencie inicjacji obiektu, więc wywołanie go w metodzie TUser:: clearData() jest błędem. Musisz odpowiedzieć sobie na pytanie jakie dane wymagane są do prawidłowego funkcjonowania obiektu User i zadbać o ich inicjalizację w czasie konstruowania obiektu. Prawdopodobnie nie może istnieć użytkownik, który się jakoś nie nazywa i nie ma podanego e-maila (który jest w wielu miejscach aplikacji wykorzystywany do komunikacji z nim). Konkretyzacja obiektu powinna wtedy wyglądać tak:

[PHP] pobierz, plaintext 
<?php
$oNewUser = new User( 'Krzysztof', 'adres@email.pl' );
?>
[PHP] pobierz, plaintext 

O enkapsulacji pamiętasz, kolejna ważna zasada zgodna z duchem obiektowości ; ). Mamy wszystkie składowe jako prywatne. Odpowiednie metody ustawiające wartości. Dodałbym jeszcze metody do pobierania potrzebnych wartości albo zrobił pożytek z metody __call() dla zmniejszenia ilości kodu (chociaż biorąc pod uwagę przejrzystość interfejsu, może jednak nie). Metody walidujące zawarł bym przy odpowiednich seterach.

Sqle do modyfikowania rekordów zgonie ze wzorcem ActiveRecord zostają w klasie User. Wyszukujące wyłączamy. Dodajemy obiekt klucza stanowiący interpretację wzorca http://martinfowler.com/eaaCatalog/identityField.html, pilnujący tożsamości obiektów i powiązanych rekordów w bazie. Przy ładowaniu obiektów z bazy rejestrujemy je w IdentityMap http://martinfowler.com/eaaCatalog/identityMap.html dostępnej za pomocą jakiegoś rejestru, zabezpieczającej naszą aplikację przed wielokrotnymi odwołaniami do bazy danych i tworzeniem wielu instancji tego samego obiektu. Na koniec dodajemy kontrolę nad modyfikacją naszych obiektów w czasie ich życia i automatyczne utrwalanie zmian w bazie danych, za co odpowiedzialnością obarczamy „obiekt jednostki pracy”. Natomiast kolekcje obiektów obsługujemy (z wykorzystaniem opóźnionego ładowania) za pomocą specjalnego obiektu implementującego interfejs jednego z iteratorów SPL (tak jak to proponuje Matt Zandstra w „php 5 Objects, Patterns, and Practice” http://www.amazon.com/exec/obidos/ASIN/159...018931-2571322)

…i czy projektowanie obiektowe nie jest poezją!? : )

Dodatkowe uwagi:
1. Obsługa błędów w Twojej klasie jest fatalna. Skoro korzystasz z PHP5 i chcesz w pełni wykorzystywać jego możliwości obiektowe, to tak jak sugerowali przedmówcy powinieneś używać wyjątków. Dobra lektura na początek: http://www.zend.com/php5/articles/php5-exceptions.php
2. Pilnuj typu zmiennych, to że luźne typowanie w php daje dużo możliwości, nie zabezpiecza go przed wieloma problemami związanymi z tym. Ułatwieniem mogą być dobrze przygotowany standardy pisania kodu. Ja na przykład jak dodaje do nazw zmiennych przedrostek wskazujący jakiego typu jest zmienna. Poza tym lepiej dokumentuj swój kod, a w szczególności API. http://phpdoc.org/
3. Dodatkowo skoro PHP5 skoro nowy model obiektowy to sugeruję PDO jako abstrakcję dostępu do danych, zamiast AdoDb.

Tak to z grubsza mogłyby wyglądać zależności klas. Tobie pozostawiam implementację i życzę miłej zabawy.

(IMG:http://blazej.niepije.org/php.pl/user.png)