[Klasa] Router, Moje pierwsze praktyczne kroki w OOP i prośba o ocenę :) Opcje

[mathev19]

Na wstępie chciałem się ze wszystkimi przywitać, ponieważ jest to mój pierwszy post.
Dobrze przejdźmy więc do rzeczy napisałem sobie klasę Routera, której kod możecie zobaczyć poniżej. Przede wszystkim proszę o obiektywną krytykę bo jest to moja pierwsza klasa. (IMG:style_emoticons/default/snitch.gif)

[PHP] pobierz, plaintext 
<?php
 
class Router {
	private $sPath = null;
	private $sController = null;
	private $sAction = null;
	private $aUrlSegments = array();
	private $aParams = array();
 
	public function __construct() {
		$this->sPath = $_SERVER['PATH_INFO'];
		$this->setUrlSegments();
		$this->setController();
		$this->setAction();
		$this->setParams();
	}
 
	/**
	 * pobiera nazwę serwera
	 * @return string
	 */
	public function getServerName() {
		return $_SERVER['HTTP_HOST'];
	}
 
	/**
	 * pobiera ścieżkę i dzieli ją na segmenty
	 * następnie umieszcza wszystko w tablicy aUrlSegments
	 */
	private function setUrlSegments() {
		$this->aUrlSegments = explode('/', $this->sPath);
	}
 
	/**
	 * ustawia nazwę controllera
	 */
	private function setController() {
		 $this->sController = $this->getUrlSegment(1);
	}
 
	/**
	 * pobiera nazwę controllera
	 */
	public function getController() {
		 return $this->sController;
	}
 
	/**
	 * ustawia nazwę akcji
	 */
	private function setAction() {
		$this->sAction = $this->getUrlSegment(2);
	}
 
	/**
	 * pobiera nazwę akcji
	 */
	public function getAction() {
		return $this->sAction;
	}
 
	/**
	 * pobiera segment URL'a o wyznaczonym id
	 * @param int $id
	 * @return string
	 */
	public function getUrlSegment($id) {
		return (isset($this->aUrlSegments[$id])) ? $this->aUrlSegments[$id] : false;
	}
 
	/**
	 * ustawia parametry z URL'a
	 */
	private function setParams(){
		$this->aParams = array_slice($this->aUrlSegments,3);
	}
 
	/**
	 * pobiera parametry z URL'a
	 */
	public function getParams(){
		return $this->aParams;
	}
 
	/**
	 * tworzy adres URL
	 * @param string $sController
	 * @param string $sAction
	 * @param array $aParams
	 * @return string
	 */
	public function createUrl($sController,$sAction,$aParams) {
		return $this->getServerName().'/index.php/'.$sController.'/'.$sAction.'/'.$aParams[0].'/'.$aParams[1].'/'.$aParams[2];
	}
}
 
?>
[PHP] pobierz, plaintext 

Teraz moje pytania dotyczące ów klasy:
1. Czy ogólny sposób napisania Routera jest zgodny z filozofią OOP?
2. Co mogę napisać inaczej (czyt. lepiej)?
3. W jaki sposób prawidłowo tworzyć URL'e (metoda createUrl, na którą jak widać w kodzie nie mam zupełnie pomysłu) (IMG:style_emoticons/default/sadsmiley02.gif)

[Crozin]

1. Daruj sobie tą notację (znowu zapomniałem jak się nazywa) z typem zmiennej w jej nazwie. Jeżeli masz mieć taki burdel w kodzie czy nazewnictwie, że nie mógłbyś się połapać w nim oznacza to, że coś masz źle napisane.
2. $this->sPath = $_SERVER['PATH_INFO']; - jedna z zasad OOP: za wszelką cenę unikać pobierania jakiś globalnych danych z wnętrza metod. Co jeżeli z jakiś przyczyn będziesz chciał określać parametry nie na podstawie tej zmiennej tylko np $abc? Normalnie nie stanowi to problemu: new Router($_SERVER['PATH_INFO']); lub new Router($abc);, a u Ciebie trzeba babrać się w kodzie klasy.
3. Nazwy metod zaczynające się na set czy get z reguły służą do oznaczania tzw. publicznych setterów i getterów - wprowadzasz w błąd. Jeżeli potrzebujesz jakiejś metody, która uruchomi się na początku działania obiektu lepiej nazwać ją setup lub setupSomething.
4. Router::getServerName() - router nie powinien zajmować się takimi rzeczami.
5. W ogóle to te metody set*() u Ciebie są na siłę wepchane. Możesz ich zawartość zbiorczo wrzucić w jedną metodę, albo bezpośrednio w kontrolerze wywołać.
6. Każda z Twoich prywatnych metod powinna być co najwyżej chroniona.

3. W jaki sposób prawidłowo tworzyć URL'e (metoda createUrl, na którą jak widać w kodzie nie mam zupełnie pomysłu)

URLe tworzy się w sposób odwrotny do pierwotnego działania routera. Na podstawie określonych parametrów tworzy się ciąg reprezentujący je. Ty masz bardzo prostu Router, który ogranicza się do rozbicia względem znaku ukośnika, a więc generowanie to łączenie parametrów (ułożonych w odpowiedniej kolejności) znakiem ukośnika.

Poza tym przy moim routerze zastosowałem wzorzec singleton, podczas jednego wywołania Router jest mi potrzebny w kilku obiektach, więc w ten sposób unikam bezsensownego powielania działania funkcji explode

No to jeszcze raz: Singleton nie służy do dostępu do obiektu z każdego miejsca aplikacji!

1. To co obecnie zrobiłeś, czyli:

   [PHP] pobierz, plaintext 
   protected function myMethod() {
       $router = Router::getInstance();
   }
   [PHP] pobierz, plaintext 

   Jest dokładnym odpowiednikiem:

   [PHP] pobierz, plaintext 
   protected function myMethod() {
       global $router;
   }
   [PHP] pobierz, plaintext 

   A o tym dlaczego global jest złem w czystej postaci zapewne wiesz. Dodałeś jedynie pseudo-obiektową otoczkę dla niego.
2. Tracisz jakąkolwiek przenośność kodu - nagle kod Twojej klasy ABC staje się zależny niepotrzebnie zależny od kodu innej klasy.
3. Jakakolwiek próba zmiany zachowania działania obiektu ABC (oczywiście tego związanego z wykorzystaniem Routera) oznacza konieczność mieszania w kodzie klasy (patrz pkt 1.2 z pierwszej listy)

Czy obiekt widoku powinienem tworzyć w dispatcherze czy jednak przerzucić to na kontroller?

Nazwa kontroler sugeruje, że mówimy tutaj o MVC, a podstawowe założenie tego wzorca to: Kontroler decyduje jakiego widok(ów) użyć i jakie przekazać do niego(ich) modele - dodatkowo Kontroler może wysłać jakąś informację do Modelu

Ten post edytował Crozin 29.07.2010, 12:50:09

[mathev19]

Przede wszystkim dzięki Crozin za tak wyczerpującą odpowiedź.

Daruj sobie tą notację (znowu zapomniałem jak się nazywa) z typem zmiennej w jej nazwie. Jeżeli masz mieć taki burdel w kodzie czy nazewnictwie, że nie mógłbyś się połapać w nim oznacza to, że coś masz źle napisane.

Jest to tzw. notacja węgierska (IMG:style_emoticons/default/winksmiley.jpg)

$this->sPath = $_SERVER['PATH_INFO']; - jedna z zasad OOP: za wszelką cenę unikać pobierania jakiś globalnych danych z wnętrza metod. Co jeżeli z jakiś przyczyn będziesz chciał określać parametry nie na podstawie tej zmiennej tylko np $abc? Normalnie nie stanowi to problemu: new Router($_SERVER['PATH_INFO']); lub new Router($abc);, a u Ciebie trzeba babrać się w kodzie klasy.

Trafna uwaga. Kompletnie nie wziąłem tego pod uwagę.

Nazwy metod zaczynające się na set czy get z reguły służą do oznaczania tzw. publicznych setterów i getterów - wprowadzasz w błąd. Jeżeli potrzebujesz jakiejś metody, która uruchomi się na początku działania obiektu lepiej nazwać ją setup lub setupSomething.

Ale jeżeli później chce pobrać pewne dane z owego Routera to metody get są poprawne. Dobrze rozumuję?

Router::getServerName() - router nie powinien zajmować się takimi rzeczami.
W ogóle to te metody set*() u Ciebie są na siłę wepchane. Możesz ich zawartość zbiorczo wrzucić w jedną metodę, albo bezpośrednio w kontrolerze wywołać.
Każda z Twoich prywatnych metod powinna być co najwyżej chroniona.

Mógłbyś rozwinąć twoją myśl? Bo nie za bardzo "łapię".

Nazwa kontroler sugeruje, że mówimy tutaj o MVC, a podstawowe założenie tego wzorca to: Kontroler decyduje jakiego widok(ów) użyć i jakie przekazać do niego(ich) modele - dodatkowo Kontroler może wysłać jakąś informację do Modelu

Jeżeli chodzi o Kontroler to powiedz mi jakiego typu informację może on wysyłać do Modelu?
Kontroler ma być pośrednikiem informacji pomiędzy Modelem a Widokiem?