Zasada jednej odpowiedzialności - krótka klasa

Question

Witam, chciałbym się poradzić w jednej sprawie. Pytanie nie jest najwyższych lotów, raczej podstawy.

Zawsze pisałem tylko proceduralnie, od jakiegoś czasu uczę się obiektówki. Zacząłem przede wszystkim od teorii, znalazłem niejednokrotnie informację, że metoda powinna być odpowiedzialna tylko za jedno zadanie. Tak więc zrobiłem sobie małą klasę i nie do końca wiem czy poprawnie wg. zasady jw. Mógłby ktoś mi to sprawdzić i ewentualnie podpowiedzieć co zmienić? Chciałbym się uczyć dobrych nawyków od samego początku.

 

Klasę stworzyłem na trzy sposoby. Chodzi mi o klasę Router/ Routertwo/ RouterThree

<?php

class Router 
{
    protected $getPost;
    
    public function __construct()
    {
        $this->getPost = new GetPost();
    }
    
    public function execute()
    {
        return $this->existsAction();
    }
    
    protected function existsAction()
    {
        if (($this->getPost->existsGet('action')) && (!$this->getPost->isNullGet('action')))
            return $this->actionGet();
        
        else if (($this->getPost->existsPost('action')) && (!$this->getPost->isNullPost('action')))
            return $this->actionPost();
        
        else return false;
    }
    
    protected function actionGet()
    {
        return $_GET['action'];
    }
    
    protected function actionPost()
    {
        return $_POST['action'];
    }
}

<?php

class RouterTwo 
{
    protected $getPost;
    
    public function __construct()
    {
        $this->getPost = new GetPost();
    }
 
    public function execute()
    {
        
        if (!$temp = $this->existsAction()) return false;
        else return $this->$temp();
    }
    
    protected function existsAction()
    {
        if (($this->getPost->existsGet('action')) && (!$this->getPost->isNullGet('action')))
            return 'actionGet';
        
        else if (($this->getPost->existsPost('action')) && (!$this->getPost->isNullPost('action')))
            return 'actionPost';
        
        else return false;
    }
    
    protected function actionGet()
    {
        return $_GET['action'];
    }
    
    protected function actionPost()
    {
        return $_POST['action'];
    }
}

<?php

class RouterThree 
{
    protected $getPost;
    
    public function __construct()
    {
        $this->getPost = new GetPost();
    }
 
    public function execute()
    {
        
        if ($this->existsActionGet()) {
            return $this->returnActionGet();
        } elseif ($this->existsActionPost()) {
            return $this->returnActionPost();
        }
            
    }
    
    protected function existsActionGet()
    {
        if (($this->getPost->existsGet('action')) && (!$this->getPost->isNullGet('action')))
            return true;
        else return false;
    }
    
    protected function existsActionPost()
    {
        if (($this->getPost->existsPost('action')) && (!$this->getPost->isNullPost('action')))
            return true;
        else return false;
    }
    
    protected function returnActionGet()
    {
        return $_GET['action'];
    }
    
    protected function returnActionPost()
    {
        return $_POST['action'];
    }
}

 

W klasie Router - w pierwszej jaką stworzyłem jest metoda actionExists, która sprawdza czy akcja wgl istnieje w post/ get jezeli tak to zwraca mi jaka to akcja, a jezeli nie to po prostu false. Czyli tutaj jakby wykonuje dwa zadania, bo sprawdza i ew. zwraca mi jaka to akcja, czy jednak można to jeszcze uznać jako jedno zadanie?

 

Przy drugiej klasie trochę zmieniłem. Tutaj natomiast ta metoda sprawdza czy istnieje akcja oraz zwraca mi czy jest to w get lub post lub false. Czyli w sumie znowu mi zwraca nie tylko czy istnieje, lecz jeszcze gdzie ona istnieje ta akcja.

 

W trzeciej klasie to bardziej rozbiłem, bo sprawdzam po kolei czy akcja istnieje w get jezeli tak to weź nazwę akcji z get i tak samo z post. Trzecia klasa wydaje mi się, że jest najbliższa zasadzie jednej odpowiedzialności, aczkolwiek wygląda ona wg. mnie też najbardziej amatorsko.

 

Jak jest wg. was? Z góry dzięki za pomoc. 

Answer 1

Szczerze? Żadna mnie nie przekonuje.

• Co to jest GetPost? Co tam się odbywa? BTW mam nadzieję, że wiesz, że metod HTTP jest o wiele więcej ;)
• No i GetPost raczej powinno być wkładane z zewnątrz a nie ot tak tworzone (Dependency Injection).
• Jeśli jakaś metoda nazywa się existsCosTam, to wręcz musi zwracać true/false.
• Co robią actionGet i actionPost? Prawdę mówiąc Router nie powinien wiedzieć o istnieniu $_GET i $_POST. Takie dane powinien dostawać z zewnątrz.
• Myślę, że router powinien po prostu zwracać jaka akcja powinna być wykonana a to, jak i kiedy zostanie wykonana, to już broszka reszty frameworka.

Osobiście machnąłbym to mniej więcej tak:

<?php
namespace Router;

class Rule
{
	private $url;
	private $method;
	private $controller;
	
	public function __construct(string $url, string $method = 'GET', callable $controller) {
		// Oczywiście w realu walidacja
		$this->url = $url;
		$this->method = $method;
		$this->controller = $controller;
	}

	public function match(string $url, string $method) {
		return $url === $this->url && $this->method === $method;
	}

	public function getController() {
		return $this->controller;
	}
}

class Router
{
	private $rules;
	
	public function __construct(array $rules) {
		foreach ($rules as $rule) {
			$this->addRule($rule);
		}
	}

	public function addRule(Rule $rule) {
		$rules[] = $rule;
	}

	public function resolve(string $url, string $method = 'GET') {
		foreach ($rules as $rule) {
			if ($rule->match($url, $method)) {
				return $rule->getController();
			}
		}
	}
}

 

Comments

Zaraz przejrzę dokładnie kod, który napisałeś. Router tylko ma mi zwracać jaka jest akcja, mainController ma robić dalej. Wrzucam resztę kodu, który zdążyłem napisać. 

 

<?php

class GetPost
{
    
    public function existsGet($name)
    {
        if ($_GET[$name] === null) {
            return false;
        } else return true;
    }
    
    public function isNullGet($name)
    {
        if ($_GET[$name] == null) {
            return true;
        } else return false;
    }
    
      public function existsPost($name)
    {
        if ($_POST[$name] === null) {
            return false;
        } else return true;
    }
    
    public function isNullPost($name)
    {
        if ($_POST[$name] == null) {
            return true;
        } else return false;
    
    }
}

 

<?php

class MainController
{
    protected $action;
    protected $router;
    
    public function __construct()
    {
        if (!$this->getAction())
            $this->doDefault();
        
        elseif (!$this->actionExists())
            $this->doDefault();
        
        else $this->doAction();
    }
    
    protected function getAction()
    {
        $this->router = new RouterThree();
        return $this->action = $this->router->execute();
    }
    
    protected function doDefault()
    {
        echo "<br>STRONA GŁÓWNA<br>";
    }
    
    protected function actionExists()
    {
        $path = $this->getPath();
        
        if (file_exists($path))
                echo "juhuuu, akcja: ".$this->action.' istnieje!';
        else echo "nope, akcja: ".$this->action.' nie istnieje!';
    }
    
    protected function getPath()
    {
        return dirname(__FILE__).'\\'.$this->action.'.php';
    }
    
    protected function doAction()
    {
        echo "ACTION";
    }
    
}

 

<?php

require_once 'view/header.html';
require_once 'view/body.phtml';
require_once 'libs/GetPost.php';
require_once 'libs/Router.php';
require_once 'libs/Routertwo.php';
require_once 'libs/Routerthree.php';
require_once 'controller/MainController.php';

$test = new MainController();

 

Pytałeś o klasę GetPost dlatego wolałem wrzucić całość

---

To tak na szybko:

• Poczytaj o PSR.
• Zwłaszcza o PSR-4 i zaimplementuj autoloading (np przy pomocy composera).
• Pierwszy raz widzę router rozwiązujący URL-e do plików. Prawdę mówiąc przy autoloadingu po prostu załączana byłaby odpowiednia klasa kontrolera i… tyle.
• Zamiast klasy GetPost zaimplementowałbym cały obiekt Request. W tej chwili bowiem Twoja klasa nie daje nic ponad $_GET czy $_POST.

---

Autoloading, już poznałem i należy to do bardziej zaawansowanego programowania, tak samo narazie zignorowałem namespace'y. Zostawiam to na później.

Strukturę kodu, nazwy klas, klamry itd. stosuję właśnie z PSR.

GetPost właśnie ma mi służyć tylko do Get i Post.

Nie rozumiem trzeciego punktu? Router ma mi tylko sprawdzić i dać nazwę akcji. Czyli jak nazwa akcji będzie np. login to maincontroller wczyta mi plik z klasą Login np. LoginController.php.

Wiem, że zamiast post i get mogę zrobić po prostu przyjazne linki, ale póki co chcę zrobić to w taki sposób

---

• Myślę, że router powinien po prostu zwracać jaka akcja powinna być wykonana a to, jak i kiedy zostanie wykonana, to już broszka reszty frameworka.

Tak właśnie stworzyłem swój kod, router zwraca jaka jest akcja, a resztą zajmuje się controller.

 

• Co robią actionGet i actionPost? Prawdę mówiąc Router nie powinien wiedzieć o istnieniu $_GET i $_POST. Takie dane powinien dostawać z zewnątrz

Czyli coś takiego: $router = new Router($_GET['action'], $_POST['action']); ??

Dlaczego nie powinienem tego użyć w klasie router?

 

• No i GetPost raczej powinno być wkładane z zewnątrz a nie ot tak tworzone (Dependency Injection).

Tutaj postaram się poszerzyć swoją wiedzę, bo nie bardzo rozumiem.

 

Musiałem wyjść, już sprawdzam Twój kod. Dzięki za odpowiedzi

 

 

---

Dla mnie troche zbyt ciężki przykład, nie widzę tworzenia instancji klasy Rule a tu nagle jest $rule->match($url, $method) i $rule->getController()

Tutaj też klasy nie widzą czy akcja znajduje się w get lub post, nie ma tego sprawdzania? Wydaje mi się, że dałeś mi zbyt zaawansowany przykład, gdy ja próbuję stworzyć najprostszą klasę z najprostszymi metodami i tylko nie mam pewności czy 'moja metoda' działa wg. zasady jednej odpowiedzialności.

Nie chcę dostać sposobu tworzenia routingu, ponieważ takie już widziałem w poradnikach MVC itp. Chcę po prostu stworzyć coś najprostszego, a potem będę to sobie zmieniać i pisać w bardziej zaawansowany sposób.

Już się dowiedziałem w jednej odpowiedzi, że existsCosTam ma mi zwracać tylko true/ false, czyli w sumie dostałem swoją odpowiedź. W pierwszej i drugiej mojej klasie Router metoda Exists... zwraca nie tylko true i false, więc sobie to poprawie. Dzięki

---

Autoloading, już poznałem i należy to do bardziej zaawansowanego programowania, tak samo narazie zignorowałem namespace'y.

IMO w dobie PHP7 to już podstawy mimo wszystko.

Strukturę kodu, nazwy klas, klamry itd. stosuję właśnie z PSR.

OK, ale PSR-4 to wciąż PSR ;)

Dlaczego nie powinienem tego użyć w klasie router?

Każdy komponent powinien być w pełni niezależny i odizolowany. Po prostu.

Tutaj postaram się poszerzyć swoją wiedzę, bo nie bardzo rozumiem.

public function __construct(GetPost $getPost)
{
    $this->getPost = $getPost;
}

Dla mnie troche zbyt ciężki przykład, nie widzę tworzenia instancji klasy Rule a tu nagle jest $rule->match($url, $method) i $rule->getController() 

Bo gotowe Rule są wkładane z zewnątrz: w tablicy do konstruktora lub pojedynczo przez Router::addRule. 

Natomiast co do Twojej klasy: dalej nie rozumiem np. sensu actionGet?

---

Ja może podlinkuje na tym etapie materiały:

https://webmastah.pl/jak-programowac-obiektowo-cz-1-wstep/

https://www.sitepoint.com/clean-code-architecture-and-test-driven-development-in-php/

https://www.youtube.com/user/PHPersPL

http://code.tutsplus.com/tutorials/solid-part-1-the-single-responsibility-principle--net-36074

I tak natrafisz na to, prędzej czy później.

---

Dzięki za odpowiedzi i linki, na pewno będą pomocne. Linki sprawdze później.

 

Co do metody actionGet, potem to nazwałem jako returnActionGet. Jeżeli okaże się, że akcja jest w get wtedy moja metoda zwróci mi jaka to jest akcja z get. Chodzi Ci o to, że nie ma sensu tworzenia metody tylko po to żeby zwróciło to co już istnieje jak własnie $_GET['action']?? Wg. mnie po prostu czytelniej jest użyć returnActionGet niż $_GET...

 

Co do odizolowania komponentów: Get i Post nie powinno się używać wewnątrz klas?

 

public function __construct(GetPost $getPost)

{

    $this->getPost = $getPost;

}

Czyli zamiast tworzyć instancję klasy w innej klasie to lepiej taką przekazać w parametrze metody? Dobrze rozumiem?

---

Co do odizolowania komponentów: Get i Post nie powinno się używać wewnątrz klas?

Raczej nie, bo zawsze może się okazać, że zechcesz przekazać dane z innego źródła (np. w trakcie testów) i jest problem. 

Czyli zamiast tworzyć instancję klasy w innej klasie to lepiej taką przekazać w parametrze metody? Dobrze rozumiem?

Zwykle w konstruktorze. Polecam poczytać o dependency injection.