Ocena mojego portfolio na wzorcu MVC

pytanie zadane 30 lipca 2018 w Nasze projekty przez Ubermade Bywalec (2,020 p.)
zmienione kategorie 30 lipca 2018 przez Comandeer

Witam! Stworzyłem ostatnio aplikację internetową na wzorcu projektowym MVC. Jest to dokładnie moje portfolio i chciałbym się spytać tych bardziej doświadczonych osób o rady i ogólną ocenę całej apliacji ;)

Hosting: https://ubermade.pl

Kod na GitHubie: https://github.com/Ubermade/MVC-Ubermade

Proszę o ocenę controllera, jak i samego front-endu ;)

1	komentarz 30 lipca 2018 przez Assasz Nałogowiec (30,460 p.) Ja tylko zwrócę uwagę na jeden szczegół - MVC to wzorzec architektoniczny, nie projektowy :)

4 odpowiedzi

odpowiedź 30 lipca 2018 przez Comandeer Guru (601,590 p.)
wybrane 30 lipca 2018 przez Ubermade

Najlepsza

NAPRAWDĘ – na prawdę to można przyrzekać.

https://github.com/Ubermade/MVC-Ubermade/blob/master/config.php – polecam zapoznać się z ideą trzymania konfiguracji w zmiennych środowiskowych.
Zamiast katalogu vendor powinieneś po prostu wrzucić composer.json.
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/templates/front/base.html.php#L6 – chrome=1 nie ma sensu, bo Chrome Frame umarło z dobre 4 lata temu.
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/styles/main.css#L50 czy https://github.com/Ubermade/MVC-Ubermade/blob/master/src/styles/main.css#L84 – nie stosuje się takich selektorów
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/controller/user.php#L28 – czemu model zawiera metodę do logowania?
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/controller/user.php#L27 vs https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/controller/user.php#L10 – dwa te same modele w jednej klasie? Wygląda to tak jakby nic nie trzymało stanu, ergo – nie było faktycznymi klasami. Poza tym takie rzeczy powinny być wstrzykiwane jako zależności.
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/controller/category.php#L6 – to powinno być wyciągnięte do zewnętrznego serwisu.
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/engine/view.php#L45 – żadnej walidacji? Poza tym – JSONp?
https://github.com/Ubermade/MVC-Ubermade/blob/master/src/app/engine/view.php#L64-L65 – polecałbym trzymać się mimo wszystko PSR-2, a przynajmniej: spójnej konwencji nawiasów klamrowych.
https://github.com/Ubermade/MVC-Ubermade/tree/master/src/app/engine/router – rozbieżność w wielkości liter w nazwie klasy i pliku = pewne problemy przy próbie przeniesienia tego poza Windows.

1	komentarz 30 lipca 2018 przez Patrycjerz Mędrzec (192,320 p.) Dusza absolwenta polonistyki się odezwała

komentarz 30 lipca 2018 przez Ubermade Bywalec (2,020 p.)

Ohoho widzę, że kolega się ładnie rozpisał :d

Spaliłem się ze wstydu, gdy mi wytknąłeś to "na prawdę". Z reguły nie popełniam błędów ortograficznych, a tutaj taki byk :v

To z vendorem poprawię w następnym commicie

Co do tych selektorów, to również poprawię przy następnym commicie... Byłem myśli, że ID należy stosować przy jednorazowych.. Nie wiebm jak to ująć :d

A czy to źle? Gdzie indziej mógłbym wsadzić to logowanie?

Rzeczywiście kiczowato to wygląda... Również poprawię ;)

Co masz na myśli z tym "wyjęte do innego serwisu"?

A jaka jest różnica między psr-4 a psr-2 poza tym "spójność konwencji"?

O tym nie pomyślałem... Również poprawię ;)

Ogólnie wielkie dzięki za to, że się tak rozpisałeś :) Warto słuchać starszych i mądrzejszych i prosiłbym Cię jeszcze o dokładniejsze opisanie rzeczy, o które się zapytałem powyżej :d

komentarz 30 lipca 2018 przez Comandeer Guru (601,590 p.)

A czy to źle? Gdzie indziej mógłbym wsadzić to logowanie?

Do osobnego serwisu.

Co masz na myśli z tym "wyjęte do innego serwisu"?

Powinna być osobna klasa zajmująca się sprawdzaniem uprawnień użytkownika.

A jaka jest różnica między psr-4 a psr-2 poza tym "spójność konwencji"?

Taka, że PSR-2 mówi o stylu pisania kodu, a PSR-4 o automatycznym wczytywaniu klas ;)

komentarz 30 lipca 2018 przez Ubermade Bywalec (2,020 p.)
edycja 30 lipca 2018 przez Ubermade

A dobrze by było, gdybym sprawdzenie logowania wsadził do kofiguracji routera? Chodzi o informację true lub false, którą bym późnej przetwarzał w kontrolerze.

komentarz 31 lipca 2018 przez Gambr Dyskutant (7,530 p.)

Sprawdzanie logowania powinno być kolejnym osobnym serwisem, do którego mógłbyś albo stworzyć helpera albo wybudować go w trzewia controllera

komentarz 1 sierpnia 2018 przez Comandeer Guru (601,590 p.)

Jak ma być osobnym serwisem, to jak może być równocześnie wewnątrz kontrolera?

komentarz 1 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)
edycja 1 sierpnia 2018 przez Gambr

Kontroler powinien wiedzieć że istnieją moduły odpowiedzialnie za konkretne zadania i je posiadać i być fasadą do komunikacji z nimi. Walidacja requesta z formularza lub właśnie auth są takimi modułami i kontroler jedynie wywołuje je do pracy bez znajomości ich implementacji.

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

@Comandeer, wybudować go w trzewia controllera

To wyrażenie było nietrafione z mojej strony. Miałem na myśli to, że kontroler będzie klientem zewnętrznego serwisu Auth i będzie go posiadać jako swoją zależność a nie implementację.

odpowiedź 30 lipca 2018 przez highui Nowicjusz (240 p.)

Nie jestem doświadczony, nie wytknę Ci błędów programsitycznych, jeszcze za mało wiem. Mogę się podzielić wrażeniami z punktu widzenia klienta.

"Oferuję niską cenę za na prawdę wysokiej jakości produkt." - Nie jesteś w stanie określić jak duże projekty dostaniesz do realizacji, a cena niestety nie idzie w parze z jakością. Wystarczy jakość.

Jak dla mnie wyświatlanie liter w kółku na środku jest słabym pomysłem, jednak podkreślam jest to moje zdanie ;)

"Witaj na mojej stronie!" - jak dla mnie jest to fajne, ale raczej w formie żartu, lepiej już zaśmieszkować z czymś ala "Hello World!" :D

Sam design jak najbardziej na plus za proste, regularne kształty, stonowaną kolorystykę, jednak odzieliłbym jakimś div'em treść od tła strony ;)

komentarz 30 lipca 2018 przez Ubermade Bywalec (2,020 p.)

Dzięki, zastosuję się do dwóch ostatnich rad przy następnym updacie strony ;)

komentarz 30 lipca 2018 przez Marcin_N_97 Stary wyjadacz (10,290 p.)

Totalnie zgadzam się z @highui.

Dodam tylko do tego 1 punktu:

* "Pewność, że strona będzie wolna od błędów oraz niedopracowań." -> w literze prawa jeśli Twoja strona będzie miała jakikolwiek błąd to n tej podstawie klient może rządać zwrot pieniędzy

* Dużo bardziej przekonuje mnie, że liczy się dla Ciebie tlyko jakośc czy coś w tym stylu niż ta niska cena

* zamiast niskiej ceny możesz rzucić coś o tym, że realizujesz też prostsze projekty dla klientów indywidualnych

komentarz 31 lipca 2018 przez Gambr Dyskutant (7,530 p.)

Ostatnia uwaga bardzo sprytna

odpowiedź 31 lipca 2018 przez Gambr Dyskutant (7,530 p.)
edycja 1 sierpnia 2018 przez Gambr

Nazwy klas powinny być wielką literą i opisywać ich przeznaczenie . Zamiast nic nie mówiącego pliku base.php z klasą base, proponowałbym BaseController.php z klasą BaseController. Jeżeli przestrzegasz konwencji w nazwach funkcji pisząc je ogólnie przyjętym camel casem, to przestrzegaj jej również w przypadku klas
Wszystkie klasy z app/engine są abstrakcyjne, mimo że nie wykorzystujesz w nich metod abstrakcyjnych. Jeżeli chcesz zablokować możliwość stworzenia instancji klasy, zwyczajnie użyj prywatnego konstruktora
Pola klasy route i routes są protected mimo że nic z nich nie dziedziczy. Nie lepiej private?
Dobrze by było stworzyć wrappera do pracy z sesją i wyrzucić session_start z index. Klasa Session miałaby metody get i set, która wykryje status sesji i w razie potrzeby ją uruchomi. Tutaj mamy wybór czy chcemy stworzyć singletona $session, czy poprostu używać metod statycznych. To już zależy od Ciebie, ale stworzenie takiej klasy jest wg mnie ważne, ponieważ metody są znacznie czytelniejsze niż $_SEESION[] , mogą też ułatwić przesyłanie danych wraz z redirectem do innej strony
Dla typowo front endowych zasobów jak js, css i zdjęcia w MVC jest osobny folder public, napewno nie powinny one znajdować się w sercu Twojego systemu, czyli w app
Kontrola dostępu, generowanie linków, zarządzanie wyjątkami z ewentualnym generowaniem wiadomości o błędach czy walidacja formularzy to szczegółowe zadania, które powinny być wykonywane przez osobne, wyspecjalizowane obiekty. Controller powinien pełnić rolę fasady do komunikacji z nimi, innaczej jego obraz zostaje mocno zamazany. Decoupling systemu, mimo że tworzy wiele klas jest dobry, ponieważ duża liczba małych komponentów jest znacznie łatwiejsza w zrozumieniu i utrzymaniu niż kontroler na 38 tysięcy lini

komentarz 1 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Co do jednego... Front-end nie znajduje się w app tylko jest w osobnym katalogu "src" w którym też jest katalog "app"... Widziałem nieraz takie rozwiązanie i pomyślałem, że będzie okej...

Co do reszty to postaram się to wprowadzić w projekt ;) Dzięki za pomoc !

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)
edycja 2 sierpnia 2018 przez Ubermade

Mam tylko jeszcze jedno pytanie co do tych dodatkowych klas (np to do sesji)

Czy mógłbym je umieścić w katalogu engine i w nim stworzyć folder o nazwie utilities czy jednak powinienem inaczej to zrobić?

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Utilities brzmi dobrze. Oddzielasz swój core od "pomocników", więc jak dla mnie ok.

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

no to już zrobiłem... czy mógłbyś zerknąć na githuba? Ogólnie wyczyściłem wcześniejsze commity bo pomieszałem kilka rzeczy :d

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Jest znacznie lepiej Kilka spraw jakie zauważyłem na szybko:

Klasa Session i Auth powinna być albo globalnie dostępnym Singletonem albo prościej, mieć tylko statyczne metody.
```
$data = Session::get('data');

//niz długie

$session = new Session();
$data = $session->get('data');
```
Sesja oraz Auth jest jedna w systemie i nie ma potrzeby tworzyć obiektu przy każdym odwołaniu
Często używasz metody generateUrl() która robi w zasadzie to samo zarówno w View i Controllerze. Dobrzy by było raz ją zaimplementować np wewnątrz routera i potem tylko się do niej odnosić bez powielania implementacji w każdej klasie która z takiej funkcjonalności korzysta
Twoje podstawowe klasy View, Model i Controller są abstrakcyjne. Jeżeli chcesz uniemożliwić tworzenie ich instancji, użyj private lub protected konstruktora. Daje Ci to sporo elastyczności, w przyszłości np kiedy podstawowy kontroler znacznie się rozbuduje i będzie mieć wiele zależności, mógłbyś użyć chronionego konstruktora do zbudowania ich i ustawienia pól klasy, które później będą odziedziczone. Taki konstruktor będzie dostępny tylko dla klas które go rozszerzają a więc innych kontrolerów, nikt z zewnątrz nie stworzy ich instancji.

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Troszku nie rozumiem tego ostatniego podpunktu... Konstruktor to jest funkcja, więc jakbym miał do niego zaimplementować resztę swoich funkcji?

Co do reszty to zabieram się do poprawy tego :)

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Jeszcze jedno pytanie... Co w przypadku access? Czy metody logged i unlogged też mogę zrobić statyczne?

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

<?php

class Controller
{
	protected function __construct()
	{
		//
	}
}

class PostsController extends Controller
{
	public function __construct(){
		parent::__construct();
	}
}

$controller = new PostsController(); //działa

//$baseController = new Controller(); bląd

Z czasem, Controller może mieć jakieś skomplikowane zależności, które będą potrzebne do jego poprawnego działania.
Wewnątrz konstruktora, będą one odpowiednio budowane, np za pomocą fabryk czy builderów, tak aby klasy rozszerzające Controller mogły z nich korzystać. Stosując takie podejście uniemożliwiamy tworzenie obiektów podstawowego kontrolera, czyli to co daje użycie klasy abstrakcyjnej, jednak umożliwiamy kontrolerom dziedziczącym odwoływać się do konstruktora rodzica, w celu np zbudowania zależności.

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Acces też powinien korzystać ze static, ponieważ jest jeden i uniwersalny na cały system i nie ma potrzeby tworzenia wielu dopasowanych obiektów klasy, a użycie statica uprości kod bo nie będzie potrzeby tworzenia instancji new Acces()

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Dalej nie mogę zrozumieć na jakiej zasadzie to ma chodzić UwU W jednym pliku ma być kilka klas, które odwołują się do jednej klasy, a z poziomu np BaseControllera mamy się odwoływać do klas w tym głównym kontrolerze?

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Nie to był tylko schemat. Chodziło mi o użycie protected constructora w tym podstawowym kontrolerze zamiast klasy abstrakcyjnej. Tylko ta zmiana. Tamte przykłady były kompletnie w oderwaniu od konkretnej implementacji i miały pokazać korzyści jakie dają ukryte konstruktory.

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

No dobrze, ale jak w funkcji construktora miałbym się odwołać do pozostałych funkcji?

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Wewnątrz konstruktora możesz przecież użyć $this->funcja(); Niezbyt rozumiem problem. Wydaje mi się, że jakoś zagmatwałem ten podpunkt o podmiance abstract na ukryty konstruktor i nie do końca sie rozumiemy nazwajem ;p

komentarz 2 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

<?php

namespace Ubermade\engine;

class Controller
{
    protected function __construct(){

    }

    public function redirect($url){
        header('location: ' . $url);
    }

    public function generateUrl($name, $data = null){
        $router = new \Ubermade\engine\router\Router('http://' . $_SERVER["SERVER_NAME"] . $_SERVER["REQUEST_URI"]);
        $collection = $router->getCollection();
        $route = $collection->get($name);
        if (isset($route)) {
            return $route->generateUrl($data);
        }
        return false;
    }
}

Czyli to miałoby wyglądać w taki sposób? A później z poziomu np BaseControllera dodatkowo musiałbym za pomocą parrenta się odwoływać?

komentarz 2 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Tak dokładnie, co prawda obecnie możesz wszystko ładnie odziedziczyć bez odnoszenia się do parent, ale gdy zależności będą skomplikowane, to bedzie bardzo przydatne. Pozatym nawet jeżeli nigdy sie nie odwołasz do tego konstruktora, to użycie abstract class bez metod abstrakcyjnych mija się z celem, więc 2:0 dla rozwiązania z ukrytym konstruktorem

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

No niestety, wywala mi taki error:

Fatal error: Uncaught Error: Class 'Ubermade\controller\parrent' not found in D:\Programowanie\xampp\htdocs\MVC-Ubermade\src\app\controller\BaseController.php:8 Stack trace: #0 D:\Programowanie\xampp\htdocs\MVC-Ubermade\index.php(12): Ubermade\controller\BaseController->__construct() #1 {main} thrown in D:\Programowanie\xampp\htdocs\MVC-Ubermade\src\app\controller\BaseController.php on line 8

komentarz 3 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Napewno używasz?

parent::__construct()

Ten przykład jaki wkleiłem wyżej działa i wszytko jest poprawnie.

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

<?php

namespace Ubermade\controller;
use Ubermade\utilities\Access;

class BaseController extends \Ubermade\engine\Controller{
    public function __construct(){
        parrent::__construct();
    }

    public function home(){
        $view = new \Ubermade\view\BasicView();
        $view->renderHTML('home', 'front/base/');
    }
    public function about_me(){
        $view = new \Ubermade\view\BasicView();
        $view->renderHTML('about_me', 'front/base/');
    }
    public function contact(){
        $view = new \Ubermade\view\BasicView();
        $view->renderHTML('contact','front/base/');
    }
    public function panel(){
        Access::logged();
        $view = new \Ubermade\view\BasicView();
        $view->renderHTML('panel', 'front/base/');
    }
}

komentarz 3 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Upewnij się ze improtuejsz najpierw podstawowy kontroler, potem swoje które z niego dziedziczą. Jeżeli Twój loader działa to kod też powinien, bo jest poprawny

komentarz 3 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

parent przez jedno 'r'

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Dobra, działa :D I zastosować to również dla View oraz Modelu?

komentarz 3 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Tak, zastanów się jednak czy view i model będą kiedykolwiek rozbudowywane o jakieś zależności i nowe serwisy? Jeżeli nie i jesteś pewny, że nigdy nie będzie potrzeby odwołać się do parent to możesz zrobić private konstruktor zamiast protected

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

A jak użyje private constructora to muszę parrentować samego constructora?

komentarz 3 sierpnia 2018 przez Gambr Dyskutant (7,530 p.)

Jak użyjesz prywatny konstruktor to odwołanie się do parent będzie niemożliwe w klasach dziedziczących

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Okej, dzięki :) To chyba na tyle, zrobię to wszystko, wrzucę na gita i podeślę do Ciebie... Mógłbym dostać jakiś kontakt do Ciebie?

komentarz 3 sierpnia 2018 przez Ubermade Bywalec (2,020 p.)

Ojej, a nie mogę zrobić czegoś takiego?

<?php

namespace Ubermade\engine;
use \PDO;

class Model{
    protected $pdo;

    private function __construct(){
        $this->connect();
    }

    public function connect(){
        try{
            $this->pdo = new PDO('mysql:host='.DATABASE_HOST.';dbname='.DATABASE_NAME, DATABASE_USER, DATABASE_PASSWORD, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
            $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }catch (DBException $e){
            echo'The connect can not create: '.$e->getMessage();
        }
    }
}

Ogólnie połączenie z bazą danych znajdowało się w publicznym construct :/