Witam

Od jakiegoś czasu interesuje się programowaniem obiektowym, ale od niedawna zdecydowałem się stworzyć coś własnego. Będzie to mój pierwszy obiektowy (przynajmniej w części) CMS. chciałbym przedstawić wam klasę artykułu do oceny. Prosze o konstruktywną krytykę

[PHP] pobierz, plaintext 
<?php
 
class Artykul {
 
//zawiera unikalny numer identyfikujący artykuł w bazie danych
private $id;
//zawiera nazwę autora tworzacego artykuł
private $autor;
//zawiera nazwę kategorii do której przyporządkowany jest artykuł
private $kategoria;
//zaiwera tytuł artykułu
private $tytul;
//zawiera wstępny tekst artykułu
private $naglowek;
//zawiera główną treść artykułu
private $tekst_artykulu;
//zawiera datę utworzenia artykułu !!Unixowy znacznik czasu
private $utworzony;
//zawiera datę ostatniej modyfikacji artykułu !!Unixowy znacznik czasu
private $zmodyfikowany;
//zawiera datę ostatniej publikacji artykułu !!Unixowy znacznik czasu
private $opublikowany;
//określa czy artykuł jest nowo utworzony, czy został wczytany z bazy danych
private $new;
 
 
 
public function __construct($id=NULL)
{
  if($id)
  {
   $this->id_art = $id;
   $db = Database::GetInstance();
   $sql = "SELECT * 
       FROM j_artykuly
       WHERE id = $id";
 
   $art = $db->select($sql);
 
   if(!$art)
    throw new Exception('Wybrany artykuł nie istnieje');
 
   $this->new = false;
   $this->set($art[0]);
 
  }
  else
   $this->new = true;
 
}
 
/**
* Metoda zwraca wartość jednego wybranego pola klasy
*
* @param name - nazwa pola w klasie, której wartość chcemy uzyskać
**/
 
public function getOne($name)
{
  if(property_exists(get_Class($this), $name))
   return $this->$name;
  else
   throw new Exception('Klasa '.get_class($this).' nie posiada pola '.$name);
}
 
/**
*
* Metoda zwraca wartości wszystkich pól klasy
*
* @return tablica z danymi wszystkich pól;
*
**/
 
public function getAll()
{
 
  $arData = array(
        'id'         => $this->id,
        'autor'        => $this->autor,
        'kategoria'     => $this->kategoria,
        'tytul'        => $this->tytul,
        'naglowek'     => $this->naglowek,
        'tekst_artykulu' => $this->tekst_artykulu,
        'utworzony'      => $this->utworzony,
        'zmodyfikowany'  => $this->zmodyfikowany,
        'opublikowany'   => $this->opublikowany,
       );
 
  return $arData;
}
 
 
/**
*
* Metoda służy do ustawienia wartości pól w klasie
*
[email="*@Param"]*@Param[/email] arData- tablica przechowująca nazwy pól i odpowiadające im wartości
*
**/
 
public function set(array $arData)
{
  foreach($arData as $key => $value)
  {
   if($key == 'id' && $this->new)
    throw new Exception("Wartość pola 'id' może być nadana tylko automatycznie");
 
   if(property_exists(get_Class($this), $key))
   {
    $this->$key = $value;
   }
   else
    throw new Exception('Klasa '.get_class($this).' nie posiada pola '.$key);
  }
}
 
 
 
/**
*
* Metoda Tworząca artykuł w bazie danych
*
**/
 
public function createArt()
{
  if($this->new)
  {
   $this->checkRequiredData();
 
   $db = Database::getInstance();
   if($db->Insert('j_artykuly', $this->getAll()) > 0);
    return true;
  }
 
  throw new Exception('Bierzący artykuł już istnieje w bazie danych. Można go tylko zaktualizować');
}
 
 
/**
*
* Metoda aktualizująca dane artykułu w bazie danych
*
**/ 
 
public function updateArt()
{
  if(!$this->new)
  {
   $this->checkRequiredData();
 
   $db = Database::getInstance();
   $arConditions['id']=$this->id;
   if($db->Update('j_artykuly', $this->getAll(), $arConditions) > 0);
    return true;
  }
 
  throw new Exception('Musisz najpierw utworzyć artykuł w bazie danych aby móc go edytować');
}
 
 
/**
*
* Metoda usuwająca artykuł z bazy danych
*
**/ 
 
public function delArt()
{
  if(!$this->new)
  {
   $this->checkRequiredData();
 
   $db = Database::getInstance();
   $arWhere = array("id = ".$this->id);
   if($db->delete('j_artykuly', $arWhere) > 0)
    return true;
  }
 
  throw new Exception('Nie można usunąć bierzącego artykułu, ponieważ nie został jeszcze zapisany w bazie danych.');
}
 
 
/**
*
* Metoda sprawdzająca czy pola, które nie mogą być puste w bazie danych, są wypełnione danymi
*
**/
 
private function checkRequiredData()
{
  $arData = array();
  $arData = $this->getAll();
  $arEmpty = array();
  foreach($arData as $key => $value)
  {
   if($key=='autor' || $key=='kategoria' || $key=='tytul' || $key=='naglowek' || $key=='utworzony')
   {
    if(!$value)
     $arEmpty[]=$key;
   }
  }
 
  if(count($arEmpty) > 0)
  {
   $empty = implode(', ', $arEmpty);
   throw new Exception('Wymagane pola '.$empty.' są puste.');
  }
 
  return true;
}
 
 
}
 
?>
[PHP] pobierz, plaintext 

Bezsensowna sprawa jak dla mnie. Po pierwsze gdzie tu przemyślenie i trzymanie się jednego sposobu pisania metod ? Skoro getOne w ładny sposób czyta czy istnieje zmienna którą ktoś podał w parametrze i ją zwraca to w getAll powinna pętle przechodzić po wszystkich zmiennych klasowych i je dodawać do tablicy zamiast ręcznie rozpisujesz - podstawa to konsekwencja. Po drugie co to jest za klasa ? to kontroler, model, jakieś pudełko czy co ? Rzucanie wyjątkami na lewo i prawo jest trochę dziwne w tym wypadku. Moim zdaniem to powinno służyć tylko do wywalania krytycznych błędów lub czegoś nie do obsłużenia. W przypadku braku zmiennej coś takiego wyrzucać ? Poza tym klasa ma możliwość dodania artykułu pod warunkiem, że przy instancjonowaniu podamy produkt już ISTNIEJĄCY. A jak baza jest pusta to nie dodamy produktu bo się nam obiekt nie utworzy ? Można by się jeszcze poczepiać tylko brakuje mi czasu.

Mozna dodawać także i nowe jeszcze nie zapisane do bazy artykuły. Do tego służy metoda CreateArt, która wykona się tylko jeśli wartość pola "new" jest równa true. Jest ono ustawiane własnie podczas tworzenia obiektu w konstruktorze. Jeśli podamy w nim id to obiekt bedzie utworzony na podstawie danych z bazy, a jeśli nie to bedzioe oznaczone własnie jako nowy.

Co do nadmiernych wyjątków, myslisz, że lepiej będzie zwracać po prostu false i dopiero przy uzywaniu takiego obiektu i zwróceniu false odpowiednio wyświetlić komunikat lub wyrzucić wyjątek?

A po co dodatkowy parametr new? ID w zupełności go zastępuje W końcu jeśli jest tam NULL, to spełnia on tę samą rolę. Sprawdzasz więc jedynie jaką wartość ma ID i w razie czego podejmujesz decyzję czy robić UPDATE czy INSERT w bazie. To sprawia, że createArt i updateArt możesz zastąpić jedną metodą. Konstruktor w zasadzie działałby podobnie. Brak id -> nowy z wartościami domyślnymi, prawidłowe id -> pobiera artykuł, nieprawidłowe id -> nowy + error. Kwestia funkcji do wyświetlania wszystkiego jest tak naprawdę sporna. Można użyć funkcji getOne, ale używanie funkcji w funkcji to zawsze minimalny narzut czasowy przy wykonywaniu. Więc patrząc od strony wydajności odwołanie się prosto do obiektu pozwala na ominięcie tworzenia zmiennych tymczasowych, odwołania do funkcji, powrotu do funkcji macierzystej, niszczenie zmiennych tymczasowych na stosie itp. Nie jest więc takie głupie jak się może wydawać bo oszczędza nam zarówno czas jak i pamięć serwera. Poza tym popatrz na getOne... Wywołujesz w niej za każdym razem funkcje i warunki. To spowalniałoby getAll za każdym razem.
Zastanawiałbym się także nad kategoria i tagi. Wszystko zależnie od tego jak masz zamiar rozwiązać to od strony bazy. Do przemyślenia jest bowiem choćby to, czy artykuł może być dodany do jednej, czy wielu kategorii. Bo jeśli do wielu to musisz nieco inaczej zapytania popisać. To samo tyczyło by ewentualnego dodania do klasy zmiennej dotyczącej tagów.

Czyli widzę ze zdania podzielone sa co do metody pobierającej wszystkie pola obiektu Na razie zostawie ją tak jak jest bo bardziej przemawia do mnie argument, że jest ona szybsza niż jakby miałą wykonywać jeszcze szereg dodatkowych funckji . 

Co do tej zmiennej new to rzeczywiscie mozna to wywalić i zrobić sprawdzanie za pomocą id. Tak zrobie i dzieki za radę. 

A jak z tymi wyjątkami? Nie powinienem umieszczać ich wewnątrz klasy?

Co do wyjątków zdania są podzielone. Puryści uważają, że zostały one stworzone do obsługi błędów i tylko tak je trzeba stosować. Inni używają jak leci, w tym do przekazywania informacji na stronie. Jest to więc tak naprawdę osobista preferencja. Ja osobiście bliższy jestem pierwszej grupie. Informacja o błędach powinna jak najmniej mówić userowi o strukturze serwisu, jego bazach itp, bo to ułatwia ewentualny atak. I dlatego obsługa błędów powinna być używana tylko w miejscach wrażliwych na wystąpienie błędu, by nie dopuścić do sytuacji jaką opisałem w zdaniu poprzednim. W kodzie, który jest pewny można sobie wyjątki darować, bo to w końcu dodatkowa funkcja sprawdzająca efekty wykonywania skryptu i w minimalny sposób go spowalnia.

Troszeczkę przerobniłem moją klasę, według porad oraz własnych pomysłow. Teraz wyszło mi coś takiego.

[PHP] pobierz, plaintext 
 
class Artykul {
 
//zawiera unikalny numer identyfikujący artykuł w bazie danych
	protected $id;
//zawiera nazwę autora tworzacego artykuł
	protected $autor;
//zawiera nazwę kategorii do której przyporządkowany jest artykuł
	protected $kategoria;
//zaiwera tytuł artykułu
	protected $tytul;
//zawiera wstępny tekst artykułu
	protected $naglowek;
//zawiera główną treść artykułu
	protected $tekst_artykulu;
//zawiera datę utworzenia artykułu !!Unixowy znacznik czasu
	protected $utworzony;
//zawiera datę ostatniej modyfikacji artykułu !!Unixowy znacznik czasu
	protected $zmodyfikowany;
//zawiera datę ostatniej publikacji artykułu !!Unixowy znacznik czasu
	protected $opublikowany;
 
 
	protected $fieldsRequired = array('autor', 'kategoria', 'tytul', 'naglowek', 'utworzony' );
 
protected $dbhandle;
 
 
 
	public function __construct($id=NULL)
	{
		$this->dbhandle = Database::GetInstance();
 
		if($id)
		{
			$this->id_art = $id;
 
			$sql = "SELECT * 
							FROM j_artykuly
							WHERE id = $id";
 
			$art = $this->dbhandle->select($sql);
 
			if(!$art)
				throw new Exception('Wybrany artykuł nie istnieje');
 
			$this->id = $id;
			unset($art[0]['id']);
			$this->setAll($art[0]);
		}
	}
 
 
 
 
 
 /**
* Metoda zwraca wartość jednego wybranego pola klasy
*
*	@param name - nazwa pola w klasie, której wartość chcemy uzyskać
**/
 
	public function get($name)
	{
		if(property_exists(get_Class($this), $name))
			return $this->$name;
		else
			throw new Exception('Klasa '.get_class($this).' nie posiada pola '.$name);
	}
 
/**
*
* Metoda zwraca wartości wszystkich pól klasy
*
* @return tablica z danymi wszystkich pól;
*
**/
 
	public function getAll()
	{
 
		$arData = array(
								'id'			 	 		 => $this->id,
								'autor' 	 	 		 => $this->autor,
								'kategoria' 	 	 => $this->kategoria,
								'tytul'  	  		 => $this->tytul,
								'naglowek'		   => $this->naglowek,
								'tekst_artykulu' => $this->tekst_artykulu,
								'utworzony'  	   => $this->utworzony,
								'zmodyfikowany'  => $this->zmodyfikowany,
								'opublikowany'   => $this->opublikowany,
							);
 
		return $arData;
	}
 
 
	public function set($key, $value)
	{			
		if($key == 'id')
			throw new Exception("Wartość pola 'id' może być nadana tylko automatycznie");
 
		if(property_exists(get_Class($this), $key))
		{
			$this->$key = $value;
		}
		else
			throw new Exception('Klasa '.get_class($this).' nie posiada pola '.$key);
	}
 
 
/**
*
* Metoda służy do ustawienia wartości pól w klasie
*
*@Param arData- tablica przechowująca nazwy pól i odpowiadające im wartości
*
**/
 
	public function setAll(array $arData)
	{
		foreach($arData as $key => $value)
		{
			if($key == 'id')
				throw new Exception("Wartość pola 'id' może być nadana tylko automatycznie");
 
			if(property_exists(get_Class($this), $key))
			{
				$this->$key = $value;
			}
			else
				throw new Exception('Klasa '.get_class($this).' nie posiada pola '.$key);
		}
	}
 
 
 
/**
*
* Metoda zapisująca lub tworząca artykuł w bazie danych
*
**/
 
	public function saveArt()
	{
		if(empty($this->id))
		{
			$this->validateFields();
 
			if($this->dbhandle->insert('j_artykuly', $this->getAll()) > 0);
				return true;
		}
		else
		{
			$this->validateFields();
 
			$arConditions['id']=$this->id;
			if($this->dbhandle->update('j_artykuly', $this->getAll(), $arConditions) > 0);
				return true;
		}
	}
 
 
/**
*
* Metoda usuwająca artykuł z bazy danych
*
**/	
 
	public function delArt()
	{
		if(!empty($this->id))
		{
			$this->validateFields();
 
			$arWhere = array("id = ".$this->id);
			if($this->dbhandle->delete('j_artykuly', $arWhere) > 0)
				return true;
		}
 
		throw new Exception('Nie można usunąć bierzącego artykułu, ponieważ nie został jeszcze zapisany w bazie danych.');
	}
 
 
/**
*
* Metoda sprawdzająca czy pola, które nie mogą być puste w bazie danych, są wypełnione danymi
*
**/
 
 
	public function ValidateFields()
	{
		$arEmpty = array();
		foreach ($this->fieldsRequired as $field)
		{
			$value = $this->get($field);
			if (empty($value))
				$arEmpty[] = $field;
		}
 
		if(count($arEmpty) > 0)
		{
			$empty = implode(', ', $arEmpty);
			throw new Exception('Wymagane pola '.$empty.' są puste.');
		}
 
		return true;
	}
 
 
?>
 
 
[PHP] pobierz, plaintext 

Dodałem pole dbhandle w którym przechowywany będzie obiekt bazy danych. Kolejne dodane pole to $fieldsRequired w którym przechowywana jest tablica z polami wymaganymi do uzupełnienia. Zmodyfikowałem tez funkcję, która ma sprawdzać czy pola te są wypełnione. Na koniec połączyłem metody createArt i UpdateArt w jedną metodę SaveArt i usunełem pole new, a zamiast niego dałem sprawdzanie po id, według wczęsniejszej porady. Jak teraz według was wyglada moja klasa?

Bieżący piszemy przez ż z kropką.
Jak komentujesz metody i zmienne, to rób to konsekwentnie i komentuj wszystkie. Zachowaj jakiś standard komentowania - np. phpDocumentator.

Poczytaj o metodach magicznych.

Komentarze będą oczywiście przy każdej metodzie, na razie ich nie dodawałem bo nie wiem czy zaraz nie bedzie trzeba i tak tego usuwac

Nie ma co się oszukiwać - jak nie napiszesz od razu to w 95% przypadków te komentarze już nigdy nie powstaną