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

[Luneth]

To createUrl bym raczej sobie w ogóle odpuścił, gdyż może być kłopotliwe w stosowaniu, skoro i tak masz funkcje ustawiające Twoje konkretne 'segmenty' (setController itd). Czy napisać inaczej - myśle, że nie, podstawowy Router to na tyle krótki kod, że tak jak jest wystarczy - przejrzyście, uporządkowanie. 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 (IMG:style_emoticons/default/smile.gif)

[mathev19]

Przede wszystkim to dzięki za odpowiedzi na moje pytania Luneth. A przechodząc do tematu to chyba podaruje sobie tą metodę "createUrl()" tak jak wspomniałeś (po co robić sobie niepotrzebnie pod górę (IMG:style_emoticons/default/snitch.gif) ). Zastosowanie singletona przy routerze i dispatcherze również rozważałem ale nie byłem tego pewien. Ty natomiast uświadomiłeś mi, że jest to jednak dobry pomysł dzięki (IMG:style_emoticons/default/biggrin.gif)

#EDIT

Mam jeszcze jedno małe pytanko (napiszę tutaj żeby nie robić bezsensownie kolejnego tematu).
Czy obiekt widoku powinienem tworzyć w dispatcherze czy jednak przerzucić to na kontroller?

Ten post edytował mathev19 29.07.2010, 11:38:16

[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

[Luneth]

Wiec co proponujesz miast singletona? Wzorzec rejestru?

[Crozin]

Rejestr? Jeszcze większe zło. Po co Ci w ogóle tutaj coś takiego?

[Luneth]

Żeby bezsensownie nie były w koło macieju wykonywane metody? A już nie patrząc jedynie na Router to przecież pojawią się inne klasy, których obiekty będą musiały 'wędrować' między sobą - singleton nie,rejestr nie, jak to rozwiążesz? wszystko przez parametr ciągle?

Ten post edytował Luneth 29.07.2010, 18:00:53

[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?

[Crozin]

singleton nie,rejestr nie, jak to rozwiążesz? wszystko przez parametr ciągle?

Przekazywanie jako argument jest bardzo dobrym sposobem. Jednak z czasem może nie wystarczyć - wtedy warto skorzystać np. z Dependency Injection czy Service Locator.

Jest to tzw. notacja węgierska

Ahhh... dzięki za przypomnienie. (IMG:style_emoticons/default/winksmiley.jpg)

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

W skrócie: jeżeli dana metoda ma być niedostępna z zewnątrz i nie masz jakiegoś konkretnego powodu by blokować jej widoczność w klasach potomnych - powinna być prywatna. Co do tego, że te metody set*() wepchałeś na siłę:

[PHP] pobierz, plaintext 
public function __construct() {
    $this->aUrlSegments = explode('/', $this->sPath);
    $this->sController =$this->aUrlSegments[1];
    $this->sAction = $this->aUrlSegments[2];
}
 
// tutaj same gettery
[PHP] pobierz, plaintext 

Nie wykonujesz tam żadnych skomplikowanych operacji - dlatego możesz bez problemu wrzucić całość do kontrolera.

Jeżeli chodzi o Kontroler to powiedz mi jakiego typu informację może on wysyłać do Modelu?

Może przekazać mu takie dane do jakich widok nie ma dostępu lub nie leżą one do zakresu działania logiki widoku.

Kontroler ma być pośrednikiem informacji pomiędzy Modelem a Widokiem?

W sieci znajdziesz w zdecydowanej większości przypadków (dla PHP) kontrolera działającego właśnie jako pośrednik (kontroler pobiera dane z modelu i przekazuje je do widoku). Natomiast poprawną implementacją MVC jest zrobienie z kontrolera takiej "swatki". Określa on, żę ten a ten widok ma użyć tego, a tego modelu.

[mathev19]

Teraz to mi ładnie wszystko wytłumaczyłeś i już "kumam" przysłowiową bazę. (IMG:style_emoticons/default/tongue.gif)

W sieci znajdziesz w zdecydowanej większości przypadków (dla PHP) kontrolera działającego właśnie jako pośrednik (kontroler pobiera dane z modelu i przekazuje je do widoku).

No właśnie. Doczytałem się, że jest to wzorzec zwany MVP. Correct?

[Luneth]

@ Crozin
Dependency Injection, ok, ale wszędzie musisz przekazywać przez parametr zmienną $container wskazującą na obiekt kontenera, jedynie aby uniknąć jednej zmiennej statycznej. Czy to ma sens?

Ten post edytował Luneth 2.08.2010, 01:48:22

[Crozin]

No właśnie. Doczytałem się, że jest to wzorzec zwany MVP. Correct?

Mniej więcej właśnie tak.

Dependency Injection, ok, ale wszędzie musisz przekazywać przez parametr zmienną $container wskazującą na obiekt kontenera, jedynie aby uniknąć jednej zmiennej statycznej. Czy to ma sens?

Tak, bo:
1) Kod nie ma jakiś zależności zaszytych w swoim wnętrzu
2) Jesteś w stanie zadecydować z czego będzie korzystać obiekt - co jest w sumie następstwem pierwszego
3) Tego przekazywania jakoś specjalnie dużo nie masz

[Luneth]

Załóżmy, że mam projekt, który realizuję w myśl MVC lub MVP, wówczas więcej tego przekazywania mam, nie sądzisz?

[Crozin]

A niby dlaczego miałoby być tego przy takim projekcie więcej? Jestem pewien, że wszystkie modele, widoki, kontrolery czy prezentery (dobrze to odmieniłem?) będą dziedziczyć po jakiś abstrakcyjnych klasach, w których to znajdzie się ten nieszczęsny fragment kodu plus ewentualnie jakieś metody-proxy.

[Luneth]

'Nieszczęsny' fragment ($this->container = $container) to jedno, ale za to przy każdym konstruowaniu obiektu o jeden parametr więcej (IMG:style_emoticons/default/smile.gif) w sumie co z tego, że będzie rozszerzać jakiegoś abstrakta? Abstrakt za daną klasę-dziecko tego parametru nie przejmie, i tak i tak musimy go przekazać przy tworzeniu obiektu. Głównym problem jest dostępność konkretnego obiektu pewnej klasy (typu prosta obsługa jednej bazy danych, router, sesje, czyli coś co się tworzy tylko raz) w każdym miejscu kodu naszej aplikacji, co niby daje nam Dependency Injection Container, jednakże w każdym miejscu zewnątrz klasy/funkcji etc, jeśli chcemy z niego skorzystać w jakiejś klasie, metodzie, czy zwykłej funkcji to już musimy przekazywac parametr.

Ten post edytował Luneth 2.08.2010, 09:05:04