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ę )

[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 

konstrukto nic szczegolnego nie robi wiec chyba zrobiles go tylko po to aby byl

w metodzie validatePassword masz chyba blad bo zawsze wywoluje metode $this->setError();

co do samej obslugi bledow to nie wiem jak dalej chesz to implementowac ale zaluzmy ze ktos ma zla nazwe uzytkownika wiec ustawiasz u error na 1 ale maila ma dobrego wic wlasciwosc is_error bedzie zmieniona na 0 (brak bledu)

no i widze niekonsekwencje alebo uzywasz wartosci liczbowych albo boolenowskich (1 czy true) zdecyduj sie wiem ze php jest bardzo tolerancyjne i odczyta 1 jako true ale to nie znaczy zeby robic takie "bledy"

Nie wiem czy masz taka metode (troche ich tam duuzo) validateUserData(), ktora wywoluje kazda z podmetod walidacji: validateEmail() itd. Jesli user ma byc dodany gdy WSZSYTKIE dane sa prawidlowe to przydadza ci sie wyjatki: gdy email jest zly validateEmail() wyrzuca wyjatek, validateUserData() go lapie i zwraca FALSE/wyjatek co oznacza ze dane usera nie sa prawidlowe.

@Kinool: Faktycznie na samej górze jest 0 1 a potem true false - poprawię to.
Co do konstruktora, to pojawi tam się jeszcze parę rzeczy, m. in. to co zostało zaproponowane w tym wątku.
Co do validatePassword to masz absolutną rację

@bonzo: dobra sugestia - skorzystam

@all: Poprosiłbym jakieś sugestie co do obiektowaości, czy są jakieś niedociągnięcia, co można inaczej zrobić, ew czego nie robić.

Dzięki za dotychczasowe uwagi

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.

[ot]Krzysztofie, gdzieś ty się chował do tej pory? Don't stop, man [/ot]

Dzięki informacjom Krzysztofa wypożyczyłem sobie książkę i ostro czytam:
"Patterns Of Enterprise Application Architecture" (tak spolszczyli tytuł ) - Architerkuta systemów zarządzania przedsiębiorstwem - Wzorce projektowe). Póki co polecam!

troszeczke nie rozumiem schematu... albo z nazewnictwem sth wrong... UserFinder szuka usera tak ? w jaki sposob ma dostep do db ?

[PHP] pobierz, plaintext 
<?php
	public function getUserInfo(){
		return $this;
	}
?>
[PHP] pobierz, plaintext 

A czemu to ma służyć?
1. Zwracasz referencję, którą klient już posiada, bo musi wywołać jakoś tą metodę...
2. Pola i tak są prywatne.

Najlepszy pomysł to utworzenie zestawu getterów, moim skromnym zdaniem.

Schemat tak jak napisałem jest tylko ogólną ilustracją. Słuszna uwaga, brakuje tam informacji o związku między klasą wyszukującą, a klasą połączenia z bazą danych. Oczywiście klasa UserFinder ma również dostęp do bazy danych i wywołuje polecenia SQL w celu wyszukania i zwrócenia obiektów użytkowników.

Związek ten powinien być oznaczony jako agregacja, ponieważ z reguły obiekt połączenia z bazą danych w konstruktorach klas AciteveRecord i Finder jest przypisywany do prywatnej właściwości i dostępny później w kontekscie klasy.

Jak ma sie sprawa jesli chodzi o pozniejszą zmianę struktury (chociazby nazw) w bazie danych ? bieganie po obiektach i zmiany ?

Active Record jak wspominałem jest najprostszym wzorcem związanym z dostępem obiektów do bazy i ściśle wiąże obiekt z bazą i jej strukturą. Zmiany w bazie danych wiążą się w tym przypadku ze zmianą kodu klasy User i UserFinder. Można i należy oczywiście zaimplementować wzorzec w taki sposób aby zmiany tego typu były jak najmniej uciążliwe.

Idąc dalej, ponieważ z założenia Rekordy Aktywne odpowiadają w pełni strukturze tabeli, nadają się doskonale do zastosowania generowania kodu samych klas.

Chociaż wzorzec jest bardzo prosty i powoduje wymieszanie logiki biznesowej, mimo to jest z powodzeniem używany. Według niektórych rewolucyjny framework do tworzenia aplikacji webowych, który w ostatnim czasie wprowadził wiele zamieszania i jest w tej chwili niezwykle popularny Ruby on Rails http://www.rubyonrails.com/, opiera swój model bardzo mocno na ActiveRecords. Jest tam generowanie kodu i nacisk na szybkie i elastyczne tworzenie aplikacji. Jak grzyby po deszczu powstają klony i porty Ruby on Rails w innych językach programowania. Nie omija to również php. Najpopularniejsze jest z pewnością Ciacho http://cakephp.org/.

P.S.
@chmolu dzięki za zachęte ...we never stop :)

Witam wszystkich.

Przeczytalem te wszystkie oszerne opisy i czas dorzucic kilka groszy od siebie.

A wiec wykorzystanie ActiveRecords jest niesamowicie wygodne dla programisty ale tez ma swoje minusy, ponieważ podczas przetwarzania w aplikacji duzych ilosci danych tworzonych jest rowniez duzo instancji obiektow, co wplywa na wymagania i wydajnosc aplikacji. Odnosnie naszego przypadku ActiveRecords nadaja sie jak najbardziej.

Odnosnie tematu, ja bym posluzyl sie metoda rozbijania "bytów" na oddzielne obiekty. Wygladalo by to tak, ze uzytkownik reprezentowany by byl przez oddziena klase zawierajaca tylko jego dane (ewentualnie z walidacja). Nie wiem czy jest w PHP5 cos takiego jak "struct", ktore by sie idealnie nadawalo. By wykonywac operacje na bazie danych (dodawanie, aktualizacje itp uzytkownika) stworzyl bym kolejna klase, ktora w swoich metodach przyjmowala by objekt uzytkownika jako parametr.
Wg. mnie to jedno z najlepszych rozwiazan jesli chodzi o zachowanie "idealnej objektowosci". Popre to tylko takim małym przykładam: Co wie uzytkownik o bazie danych ?.

To takie moje skromne zdanie w tym topku.

Pozdrawiam.

Cos za cos niestety. Samego kodu za duzo zmieniac tak naprawde nie trzeba. Mozna sobie napisac cos ala genericObject czy typowy obiekt i po tym dziedziczyć poprostu. Cala klase z pierwszego posta mozna by powaznie skrocic korzystajac z tego co jest ponizej ( to tylko wzor pisany z palca, nie testowany czy cos )

[PHP] pobierz, plaintext 
<?php
 
abstract class genericObject extends Object 
{
	private 
		$myFields, $databaseFields, $loaded, $modified, $tupleId, $databaseTable, $keyName ;
 
	public function __construct($tupleId=0)
	{	
		$this->loaded=0;
		$this->tupleId=intval($tupleId);
	}
 
	public function setDatabaseParams($table,$key)
	{
		$this->databaseTable=$table;
		$this->keyName=$key;
	}
 
	public function forceLoaded()
	{
		$this->loaded=1;
	}
 
	public function getField($name)
	{	
		if($this->loaded==0)
		{
			$this->load();
		}
 
		return $this->databaseFields[$name];
	}
 
	public function getFields()
	{
		if($this->loaded==0)
		{
			$this->load();
		}
 
		$fields=func_get_args();
 
		$ret=array();
 
		foreach ($fields as $field)
		{
			if(isset($this->databaseFields[$field])) $ret[$field]=$this->databaseFields[$field];
		}
 
		return $ret;
	}
 
	public function getAllFields()
	{
		if($this->loaded==0)
		{
			$this->load();
		}
 
		return $this->databaseFields;		
	}
 
	public function setField($name,$value)
	{
		$this->myFields[$name]=$value;
		$this->modified=1;
	}
 
	public function setFields($data)
	{
		if(is_array($data))
		{
			foreach ($data as $key=> $value)
			{
				$this->setField($key,$value);
			}
		}
	}
 
	public function destroy()
	{
		if($this->tupleId)
		{
			$db=db::instance();
 
			$db->execute("DELETE FROM ".$this->databaseTable." WHERE ".$this->keyName."=".$this->tupleId);
 
			if($db->affectedRows()>0)
			{
				$this->loaded=0;
				return true;
			}
			return false;
		}
	}
 
	public function getId()
	{
		return $this->tupleId;
	}
 
	private function load()
	{
		$db=db::instance();
 
		$this->databaseFields=$db->getRow("SELECT * FROM ".$this->databaseTable." WHERE ".$this->keyName."=".$this->tupleId);
 
		$this->loaded=1;
 
		$this->modifiedFields=array();
 
		$this->modified=0;
	}
 
	public function save()
	{
		$db=db::instance();
 
		if($this->tupleId>0)
		{
			$db->update($this->databaseTable,$this->myFields,$this->keyName."=".$this->tupleId);
 
			$ret=$db->affectedRows();
		}else
		{
			$insertId=$db->insert($this->databaseTable,$this->myFields);
			$ret=$insertId;
			$this->tupleId=$insertId;
		}
 
		$this->databaseFields=$this->myFields;
		$this->databaseFields[$this->keyName]=$this->tupleId;
 
		$this->myFields=array();
 
		return $ret;
	}
}
 
?>
[PHP] pobierz, plaintext