Prośba o sprawdzenie klasy Opcje

[Jarod]

W książce natrafiłem na przykład, który wydaje mi się błędny. Wiem, że książki helionu mają literówki i różnego rodzaju błędy (na stronie helionu w erracie nie ma poprawki do tego), dlatego proszę o sprawdzenie poniższego kodu.

[PHP] pobierz, plaintext 
<?php
interface Validator{
	abstract function validate();
}
?>
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
<?php
require_once('interface.Validator.php');
 
abstract class PropertyObject implements Validator {
 
	protected $propertyTable = array();	 //Przechowuje pary nazwa-wartość przypisujące
											//własności do pól bazy danych
	protected $changedProperties = array(); //Lista własności które zostały zmodyfikowane
	protected $data;						//Dane z bazy
	protected $errors = array();
 
	public function __construct($arData) {
		$this->data = $arData;
	}
 
	function __get($propertyName) {
		if (!array_key_exists($propertyName, $this->propertyTable))
			throw new Exception('Błędna własność '.$propertyName.'!');
 
		if (method_exists($this, 'get' , $propertyName)) {
			return call_user_func(array($this, 'get' , $propertyName));
		}
		else { 
		return $this->data[$this->propertyTable[$propertyName]];
	  }
	}
 
 
	function __set($propertyName, $value) {
		if (!array_key_exists($propertyName, $this->propertyTable))
			throw new Exception('Błędna własność '. $propertyName.' !');
 
		if (method_exists($this, 'set' , $propertyName)) {
			return call_user_func(array($this, 'set' , $propertyName), $value);
		}
		else { 
			if ($this->propertyTable[$propertyName] != $value &&
			   !in_array($propertyName, $this->changedProperties)) {
			   $this->changedProperties[] = $propertyName;
			}
 
		$this_>data[$this->propertyTable[$propertyName]] = $value;
		}
	}
 
	function validate() {
 
	}
}
?>
[PHP] pobierz, plaintext 

I teraz ten kod gdzie podejrzewam, że jest błąd..

[PHP] pobierz, plaintext 
<?php
reguire_once ('class.PropertyObject.php');
 
class Address extends PropertyObject {
 
	function __construct($addressid) {
		$arData = DataManager::getAddressData($addressid);
 
		parent::__construct($arData);
 
		$this->propertyTable['addressid'] = 'adres_id';
		$this->propertyTable['id'] = 'adres_id';
		$this->propertyTable['entityid'] = 'jednostka_id';
		$this->propertyTable['address1'] = 'sadres1';
		$this->propertyTable['address2'] = 'sadres2';
		$this->propertyTable['city'] = 'smiasto';
		$this->propertyTable['zipcode'] = 'skod';
		$this->propertyTable['type'] = 'styp';
	}
 
 
   function validate () {
   
	   if (strlen($this->zipcode) != 6) {
	   $this->errors['zipcode'] = 'Należy podać poprawny kod pocztowy.';
	   } 
 
	   if ($this->address1) {
	   $this->errors['address1'] = 'Adres to pole wymagane';
	   } 
 
	   if ($this->city) {
	   $this->errors['city'] = 'Miasto to pole wymagane';
	   } 
	   
	   if (sizeof($this->errors)) {
		 return false;
	   } else {
		 return true;
	   }
 
   }
}
?>
[PHP] pobierz, plaintext 

Wydaje mi się, że błąd jest w metodzie validate(). Autor odwołuje suię w ten sposób

[PHP] pobierz, plaintext 
<?php
if ($this->address1) {
	   $this->errors['address1'] = 'Adres to pole wymagane';
	   }
?>
[PHP] pobierz, plaintext 

a powinno być chyba

[PHP] pobierz, plaintext 
<?php
if ($this->propertyTable['address1']) {
	   $this->errors['address1'] = 'Adres to pole wymagane';
	   }
?>
[PHP] pobierz, plaintext 

jeśli nie to dlaczego właśnie tak?

[envp]

Zauważ, że masz funkcje __get() oraz __set() - pierwsza wykorzystywana jest wtedy kiedy robisz

[PHP] pobierz, plaintext 
<?php
echo $obiekt->cos;
?>
[PHP] pobierz, plaintext 

druga kiedy:

[PHP] pobierz, plaintext 
<?php
$obiekt->cos = 'lalalal';
?>
[PHP] pobierz, plaintext 

Nawet, kiedy pole cos nie istnieje. Opisalem to tutaj:
envp's devblog

[Ludvik]

Nie podoba mi się ten kod. Po pierwsze właściwość nie jest walidatorem, co najwyżej może być walidowana (czyli raczej Validable). Poza tym nie rozumiem, po co deklarować metodę validate, w klasie PropertyObject... Jest to typowy przykład zastosowania metody abstrakcyjnej, a nie bezsensownego wklepywania pustej...

Tak poza tym, to dlaczego my mamy to sprawdzać? Więcej się nauczysz, jak sam będziesz chciał to sprawdzić... Odpal skrypt, zdebuguj go xdebugiem czy czymś innym...

[Jarod]

Nie podoba mi się ten kod. Po pierwsze właściwość nie jest walidatorem, co najwyżej może być walidowana (czyli raczej Validable). Poza tym nie rozumiem, po co deklarować metodę validate, w klasie PropertyObject... Jest to typowy przykład zastosowania metody abstrakcyjnej, a nie bezsensownego wklepywania pustej...

Coraz częściej dochodze do wniosku, że ta książka jest bardzo zamotana. Chyba tylko po to, żeby więcej stron było.

Zauważ, że masz funkcje __get() oraz __set() - pierwsza wykorzystywana jest wtedy kiedy robisz

Ok. Powoli łapie. Ale nie mogę zrozumieć tego kawałka kodu (z metody __get() ):

[PHP] pobierz, plaintext 
<?php
if (method_exists($this, 'get' , $propertyName)) {
			return call_user_func(array($this, 'get' , $propertyName));
		}
		else { 
		return $this->data[$this->propertyTable[$propertyName]];
?>
[PHP] pobierz, plaintext 

Po co sprawdza się czy metoda istnieje. Nie łapie też argumentów tej metody oraz call_user_func. Czytałem o tych funkjach ale nie mogę załapać. Możecie mi to wytłumaczyć?

[Ludvik]

Przepisałeś błędnie niektóre wywołania metod. Zamiast kropki (operator łączenia łańcuchów znakowych) wstawiłeś przecinek.

[PHP] pobierz, plaintext 
<?php
method_exists($this, 'get' . $propertyName)
?>
[PHP] pobierz, plaintext 

call_user_func" title="Zobacz w manualu php" target="_manual... Manual na pewno Ci wyjaśni znaczenie parametrów.
Całe to zamieszanie jest po to, aby korzystać z metod getNazwaWłaściwości zamiast bezpośredniego odwoływania się do tablicy z danymi. Jeżeli nie istnieje metoda pobierająca dane, wtedy zwracana jest wartość bezpośrednio z tablicy...

[Jarod]

Przepisałeś błędnie niektóre wywołania metod. Zamiast kropki (operator łączenia łańcuchów znakowych) wstawiłeś przecinek.

[PHP] pobierz, plaintext 
<?php
method_exists($this, 'get' . $propertyName)
?>
[PHP] pobierz, plaintext 

Tak mam w książce. Ale naet jak powinna być kropka to nie ma sensu zapis $this.get.propertyName.
Nie powinno być:

[PHP] pobierz, plaintext 
<?php
method_exists($this->'get'->$propertyName)
?>
[PHP] pobierz, plaintext 

?
Ogólnie w manulu piszą, że funkcja method_exists pobiera dwa parametry: instancje klasy, metode.

Nie łapie tego. Zresztą przecież jak mamy metode magiczną __get() to nie odwołujemy się do niej przez $this->cos tylko $this->cos();

call_user_func" title="Zobacz w manualu php" target="_manual... Manual na pewno Ci wyjaśni znaczenie parametrów.

Sprawdzałem. Podobnie jak w z method_exists nie rozumiem po co w środek wstawiane jest to get i array..

Całe to zamieszanie jest po to, aby korzystać z metod getNazwaWłaściwości zamiast bezpośredniego odwoływania się do tablicy z danymi. Jeżeli nie istnieje metoda pobierająca dane, wtedy zwracana jest wartość bezpośrednio z tablicy...

Jaki jest sens? Przecież pisząć aplikacje pisze metodę, która pobiera dane, bo wiem, że tego będę potrzebował. I nie ma sensu mówić, że nad proejktem może pracować wiele osób. Bo nawet w takim przypadku jeden programista pisze i kończy daną klase i powinien takie metody dostępowe utworzyć..

[Ludvik]

Powinno być

[PHP] pobierz, plaintext 
<?php
method_exists($this, 'get' . $propertyName)
?>
[PHP] pobierz, plaintext 

Pierwszy argument to instancja klasy. Drugi argument ('get' . $propertyName) to nazwa metody. Nie widzę nic dziwnego w tym wywołaniu...

Dalej...

[PHP] pobierz, plaintext 
<?php
call_user_func(array($this, 'get' . $propertyName));
?>
[PHP] pobierz, plaintext 

Tak samo... Pierwszy argument ma być typu callback. Jest to nazwa funkcji albo w przypadku wywoływania metod tablica. Pierwszy elementem tablicy jest referencja do obiektu (albo nazwa klasy, jeżeli wywołujemy statyczną metodę). Drugi element to nazwa metody.

Oni to zrobili po to, aby przeciążyć "magiczne" metody, ale nie tracić zachowania metod programisty getXXX... Jeżeli istnieje metoda getWlasciwosc, a ty poprosisz o dane tak:

[PHP] pobierz, plaintext 
<?php
echo $obj->wlasciwosc;
?>
[PHP] pobierz, plaintext 

To i tak efektem będzie wywołanie metody getWlasciwosc...

[Jarod]

Powinno być

[PHP] pobierz, plaintext 
<?php
method_exists($this, 'get' . $propertyName)
?>
[PHP] pobierz, plaintext 

Pierwszy argument to instancja klasy. Drugi argument ('get' . $propertyName) to nazwa metody. Nie widzę nic dziwnego w tym wywołaniu...

$this nie jest instancją klasy(?) getNazwaMetody samo w sobie nie jest dziwne. Dziwne jest to, że w przykładzie, który podałem na samym początku nie ma metody. Jest tylko magiczna (czyli jak dobrze przeczytałem wywoływana automatycznie gdy ktoś chce wywołać nieistniejącą metode), nie ma żadnej innej metody.

Dalej...

[PHP] pobierz, plaintext 
<?php
call_user_func(array($this, 'get' . $propertyName));
?>
[PHP] pobierz, plaintext 

Tak samo... Pierwszy argument ma być typu callback. Jest to nazwa funkcji albo w przypadku wywoływania metod tablica. Pierwszy elementem tablicy jest referencja do obiektu (albo nazwa klasy, jeżeli wywołujemy statyczną metodę). Drugi element to nazwa metody.

Oni to zrobili po to, aby przeciążyć "magiczne" metody, ale nie tracić zachowania metod programisty getXXX... Jeżeli istnieje metoda getWlasciwosc, a ty poprosisz o dane tak:

[PHP] pobierz, plaintext 
<?php
echo $obj->wlasciwosc;
?>
[PHP] pobierz, plaintext 

To i tak efektem będzie wywołanie metody getWlasciwosc...

Thx.

[Ludvik]

$this jest referencją do obiektu, którego metoda jest wywoływana, czyli po ludzku - do samego siebie. Przy odczycie/przekazywaniu nie różni się niczym od innych referencji.

[Jarod]

Analizuje ten przykład jeszcze raz i mam wrażenie, że autor troszke zamieszał. Czy metoda __get nie powinna raczej wyglądać tak:

[PHP] pobierz, plaintext 
<?php
function __get($propertyName) {
 
	//Jeśli istnieje metoda to ją wywołujemy
	if (method_exists($this, 'get' , $propertyName))
	{
			return call_user_func(array($this, 'get' , $propertyName));
	}
	else
	{   //Jeśli nie istnieje to sprawdzamyczy podane propertyName jest w tablicy. A nie sp
rawdzamy
		//tego na samym początku bo to bez sensu.. 
		if (!array_key_exists($propertyName, $this->propertyTable))
			throw new Exception('Błędna własność '.$propertyName.'!');
		else return $this->data[$this->propertyTable[$propertyName]];
	}
}
?>
[PHP] pobierz, plaintext 

Po co sprawdzać czy istnieje propertyName przed sprawdzeniem istnienia metody? Po to się to sprawdza (chyba) czy można zwrócić wartość z tablicy propertyTable..?

[Ludvik]

Tak też może być. Autor klasy zakładał, że nawet w przypadku istnienia metody dostępowej, należy sprawdzić czy dane istnieją w tablicy. Nie ma danych - metoda nie ma sensu...

U Ciebie istnieje możliwość wywołania metod get* nawet w przypadku, gdy dane w tablicy nie istnieją...

Kod metody __set też mi się nie podoba...

[Jarod]

Tak też może być. Autor klasy zakładał, że nawet w przypadku istnienia metody dostępowej, należy sprawdzić czy dane istnieją w tablicy. Nie ma danych - metoda nie ma sensu...

Po co? Jak istnieje metoda to ją wykonujesz a nie pobierasz wartość z tablicy. Jak napiszesz

[PHP] pobierz, plaintext 
<?php
__get(Costam)
?>
[PHP] pobierz, plaintext 

to sprawdzane jest czy istniej metoda getCostam, jeśli tak ją wywołujesz a jeśli nie to to zwraca

[PHP] pobierz, plaintext 
<?php
$this->data[$this->propertyTable['Costam']]
?>
[PHP] pobierz, plaintext 

U Ciebie istnieje możliwość wywołania metod get* nawet w przypadku, gdy dane w tablicy nie istnieją...

Jak wyżej.

[Ludvik]

Po co? Jak istnieje metoda to ją wykonujesz a nie pobierasz wartość z tablicy.

Nie odpowiem na pytanie po co - bo nie jestem autorem tej klasy. Sam bym sprawdzał dopiero w przypadku braku metody dostępowej...

[Jarod]

Nie odpowiem na pytanie po co - bo nie jestem autorem tej klasy. Sam bym sprawdzał dopiero w przypadku braku metody dostępowej...

Ale sam napisałeś:

U Ciebie istnieje możliwość wywołania metod get* nawet w przypadku, gdy dane w tablicy nie istnieją...

A to nieprawda - starałem się to wytłumaczyć w poscie wyżej.. :/

[Ludvik]

Mylisz się. Przeanalizuj dobrze kod. Żeby wywołać metodę get* musi być spełniony warunek (w którym masz błąd składniowy, o którym już wcześniej pisałem...):

[PHP] pobierz, plaintext 
<?php
if (method_exists($this, 'get' . $propertyName))
?>
[PHP] pobierz, plaintext 

Czyli musi istnieć odpowiednia metoda. Nie ma tutaj żadnego sprawdzenia, czy metoda odwołuje się do istniejącego pola. To zadanie zostało powierzone właśnie metodzie dostępowej, a nie __get... Tak na prawdę fakt posiadania przez klasę metody getDziwnePole nie gwarantuje w Twoim przypadku posiadania tego pola...

[Jarod]

Czyli musi istnieć odpowiednia metoda. Nie ma tutaj żadnego sprawdzenia, czy metoda odwołuje się do istniejącego pola. To zadanie zostało powierzone właśnie metodzie dostępowej, a nie __get...

Ok - zgadza się. Ale skąd wiesz, że metoda dostępowa tego nie sprawdza? Nie ma napisanego kodu metody dostępowej ale to chyba ona powinna sprawdzać i ewentualnie wyrzucić wyjątek.

Tak na prawdę fakt posiadania przez klasę metody getDziwnePole nie gwarantuje w Twoim przypadku posiadania tego pola...

Masz racje. Ale to metoda getDziwnePole powinna sprawdzić czy jest takie pole i jeśli jest to się wykonać a jeśli nie to throw...

Moim zdaniem autor mota bardzo. Pozatym zaglębiając się bardziej w przykłady nie jestem przekonany czy tą książkę pisali experci..

[Ludvik]

Ok - zgadza się. Ale skąd wiesz, że metoda dostępowa tego nie sprawdza? Nie ma napisanego kodu metody dostępowej ale to chyba ona powinna sprawdzać i ewentualnie wyrzucić wyjątek.

Powinna, ale zbytnie zaufanie do programisty stwarza dodatkowe niebezpieczeństwo powstania błędów w kodzie. Co nie zmienia faktu, że ten błąd wyjdzie przy testach i jest raczej niegroźny, a wklepanie tego warunku spowoduje lekką stratę wydajności.

[Jarod]

(..)a wklepanie tego warunku spowoduje lekką stratę wydajności.

A waruenk przed sprawdzaniem nie wpływa na wydajność? Będzie to samo.. Chyba że się myle..


EDIT: Dręczy mnie ejszcze jedna sprawa na którą nie uzyskłame odpowiedzi.
Wydaje mi się, że błąd jest w metodzie validate(). Autor odwołuje suię w ten sposób

[PHP] pobierz, plaintext 
<?php
if ($this->address1) {
	   $this->errors['address1'] = 'Adres to pole wymagane';
	   }
?>
[PHP] pobierz, plaintext 

a powinno być chyba

[PHP] pobierz, plaintext 
<?php
if ($this->propertyTable['address1']) {
	   $this->errors['address1'] = 'Adres to pole wymagane';
	   }
?>
[PHP] pobierz, plaintext 

jeśli nie to dlaczego właśnie tak?

Ten post edytował J4r0d 21.08.2006, 22:23:15