Witam.

Ostatnio w ramach nauki Gita i programowania obiektowego w php, napisałem sobie taką podstawę frameworka/CMSa (jak zwał tak zwał). Chciałbym prosić o sprawdzenie mojego kodu i konstruktywną krytykę.
Czy taki kod nadaje się do pokazania pracodawcy?

Głównie prosiłbym o sprawdzenie i ewentualne poprawienie mnie z:
- wyrzucanie/wyłapywanie wyjątków - czy dobrze to robię, może w złym miejscu wyrzucam/wyłapuję ?
- główny plik index.php który wszystkim kieruje - czy czegoś tam nie powinno być może? Czy prawidłowo zrobiłem wyłapując tam wyjątki przy wywoływaniu akcji kontrolera?
- routing
- struktura plików
- instancje w klasach - czy gdzieś może powinna być instancja a jej nie ma, lub gdzieś nie powinno jej być zdecydowanie?

Wiem, że tu prawie nic nie ma i wielu rzeczy brakuje, ale to tylko w ramach nauki

https://github.com/adrianpl20/oop

zapoznaj się z: PSR-0...4, przede wszystkim konwencja pisania m.in. takZmienneIKlasy a_nie_tak i wstawianie klamer itd. autoload. Użycie Composera by było miłe.

Apache? Oprócz blogów na Wordpressie, chyba już nikt tego nie używa.

foldery View i Views - takie nazywanie nie jest dobre.

W plikach .php spodziewam się kodu php a nie SAMEGO html

[PHP] pobierz, plaintext 
  public static function factory($name)
  {
    return new View($name);  
  }
[PHP] pobierz, plaintext 

nie przejdzie

W ogóle za dużo metod statycznych, route wygląda trochę jak zbiór helperów.

[PHP] pobierz, plaintext 
    // wyswietlam widok
[PHP] pobierz, plaintext 

Komentarze po polsku też średnio wyglądają.

[PHP] pobierz, plaintext 
    private function __construct()
    {
    }
[PHP] pobierz, plaintext 

?

O ile dobrze dojrzałem, łapiesz wyjątek i obsługujesz go pokazując surowy błąd użytkownikowi, to też średnio.

[PHP] pobierz, plaintext 
    public function matchRoute()
[PHP] pobierz, plaintext 

to mógłbyś jakoś sensownie cache'ować, po co milion razy to wykonywać.

Co byś odpowiedział pracodawcy na pytanie co robi klasa "class Model_Auth" jednym zdaniem? Nasuwa się niestety odpowiedź "dużo różnych dziwnych rzeczy". I czemu metody z "class Model_User" akurat nie wrzuciłeś do tego bajzlu?


- to dużą czy małą?

Ogólnie wygląda to jak zwykły kod strukturalny tylko wrzucony do klas. Dobrze, że próbujesz i z czasem po 10 takich CMSach powinno być dużo lepiej. Postudiuj koniecznie jakieś obiektowe frameworki, nawet napisz coś w nich to wymusi na Tobie i nauczy Cię pisania obiektowo. Musisz zacząć myśleć obiektowo, a widać, że myślisz strukturalnie.
Czy nadaje się do pokazania pracodawcy? Tak. Bo coś kombinujesz i pewnie na wiele moich krytyk masz jakieś mniej lub bardziej sensowne uzasadnienie.

Dzięki za odpowiedź.


Zerknąłem szybko na te PSR-0...4, i te przestrzenie nazw nie bardzo mi "podchodzą" - nie musiałem nigdy używać, kilka razy na szybko przeczytałem jakiś tutek o nich ale nie łapię ich ;d
Zły mam autoloader? Nie powinno być sprawdzanie kilku różnych "zestawów" klas w jednym loaderze?


We frameworku Kohana sporo takich metod statycznych jest Co do route to trochę tak, a co powinienem w takim przypadku zrobić z tym?


Chodzi Ci o to $e->getMessage() w app/modules/database.php ?


Czasami mam problem z doborem odpowiedniej klasy, modelu dla danej metody, troche od autoryzacji, ale i usera dotyczy... eh


Któreś klasy/pliki szczególnie masz na myśli? Może chodzi Ci o tą klasę od sesji? Nie widzę sensu zapisywać wszystkiego co się da wyciągnąć przez php w zmiennych tak jak to chyba każdy framework robi (może za mało doświadczenia mam by ujrzeć sens takiego postępowania).

Co do tego:

[PHP] pobierz, plaintext 
private function __construct()
        {
        }
[PHP] pobierz, plaintext 

Chciałem w ten sposób zablokować możliwość użycia new nazwaKlasy(); - w ramach Singletona. (Tak własnie we frameworku Kohana jest zrobione)


Problem w tym, że od roku piszę we frameworku Kohana Sporo nawyków właśnie z tego frameworka mam (np. znak _ w nazwach klas i metod).

Nie bardzo rozumiem czemu nie piszę obiektowo, co robię źle że akurat piszę, myślę strukturalnie a nie obiektowo. Widzę klasy i metody to myślę, że piszę już obiektowo, ale czemu tak nie jest?


W pliku index.php mam zrobione wyłapywanie wyjątków w ten sposób:

[PHP] pobierz, plaintext 
try
  {
    // Router
    $router = Router::instance($_SERVER['REQUEST_URI']);
    $router->matchRoute();
    // ----
 
    $get_controller = basename($_GET['controller']);
    $get_action = basename($_GET['action']);
 
 
    $tmp = 'Controller_'.ucfirst(strtolower($get_controller));
    $controller = new $tmp();
 
    $controller->before();
    $tmp = 'action_'.strtolower($get_action);
    $controller->$tmp();
    $controller->after();
  }
  catch(Exception $e)
  {
    echo $e->getMessage();
  }
[PHP] pobierz, plaintext 

Czy jest to poprawne? Czy może zamiast wyłapywania tutaj, powinienem wyłapywać w kontrolerach z wykonywanych metod z Modelu lub z jakichś innych operacji w kontrolerach?



Jeszcze raz dzięki za odpowiedź.

Głównie chodzi o konwencję, u Ciebie muszę zajrzeć w ten autoloader i zobaczyć jak i gdzie mam pakować swoje klasy, przy takim małym projekcie to jeszcze pół biedy, ale przyszłościowo przy większych projektach to utrudnienie. A tak wrzucasz sobie wg PSR-0 czy tam PSR-4 i wszystko wiadomo. Niekorzystanie z przestrzeni nazw, dla mnie grzech ciężki i niewybaczalny.


Hmm ale to nie jest OOP, to jest klasa przechowująca metody statyczne. Jeżeli nie tworzysz obiektu, a nie tworzysz to nie ma nic wspólnego z OOP. Możesz choćby przejrzeć tę klasę: http://api.symfony.com/2.0/Symfony/Compone...ing/Router.html

Kohana/CodeIgniter nie jest dobrym frameworkiem do nauki OOP. Symfony2/Laravel/Zend2/Phalcon - w te bym uderzał


Gdzieś rzucałeś echo tylko z tym błędem, nie pamiętam już gdzie to było.


Raczej musisz myśleć w drugą stronę, masz obiekt samochód i myślisz co by się przydało np zmieńKolor, sprawdźKolor, odpal, zgaś. A nie tak jak ty myślisz, że chcesz sprawdzić kolor, ale masz samochód i truskawkę, więc stworzysz klasę samochodowoTruskawkowaKlasa bo chcesz sprawdzić kolor truskawki i samochodu, to jest złe myślenie. Wiadomo, musisz później rozważyć czy może obie klasy nie powinny dziedziczyć po klasie cośCoMaKolor, gdzie będziesz już miał metodę sprawdźKolor itd. Ale nie możesz mieszać dwóch różnych klas i ładować trochę metod pod samochód, trochę pod truskawkę.


Np ta: oop/app/model/auth.php

Akurat klasę sesji masz napisaną sensownie.


Jeśli tak, to ok pomysł dobry, ale wtedy musiałbyś zadeklarować tę klasę jako final, żeby ktoś sobie nie podziedziczył i Ci jej nie nadpisał.


Im dłużej będziesz się uczył złych nawyków, tym trudniej będzie Ci w poważnej pracy się ich oduczyć.


W skrócie, tworzysz sobie potrzebne funkcje, pakujesz je do pudła i nazywasz to klasą z metodami. Oczywiście nie wszystko masz źle i w ogóle idź się powieś, np klasa sesji jest OOP, choć ktoś by się pewnie przyczepił do singletona, akurat ja nie. I na pewno jeszcze sporo sensownie napisanego kodu mógłbym tutaj wymienić, ale więcej skorzystasz czytając co jest nie do końca dobrze napisane. Mnie najbardziej boli ten routing i model_auth.



Nie no, nie będziesz przecież dla każdego kontrolera wyłapywał błędnego routingu, czy sobie to przeniesiesz czy zostawisz tutaj, to już jak uważasz. Nie ma tutaj dużo logiki więc możesz tu trzymać myślę. O ile dobrze zrozumiałem pytanie.

Ja nawet do tego go nie używam

Co do kodu, to poczepiam się
- PSR 0-4 tak jak mówił kolega @Damonsson, od konwencji nazewnictwa, przez klamry, po autoload Composer. Dlaczego? Wygoda.
- Używanie OR i AND. Zemści się to na Tobie. Poczytaj o mocy operandów.
- lib/html.php - Już lepiej wrzucić to do jakiegoś pliku helpers.php jako zwyczajne funkcje.
- lib/session.php - get i destroy będą rzucać błędami. Nie masz sprawdzania czy klucz istnieje. Można dodać ew. domyślny parametr jak nie znajdzie klucza, przydaje się często. Można jeszcze kilka metod dopisać np. has,
- lib/route.php i lib/router.php - bad idea. Nazwy będą się mylić. route.php - znowu brak sprawdzania kluczy przy pobieraniu.
- bootstrap.php - a czemu nie routes.php skoro są tam deklaracje adresów?
-

wystarczy samo is_array() i znowu AND
- View.php

Po co ten else? Nie uważasz że to jest czytelniejsze?

Dalej...

Tutaj tak na prawdę zostaje tylko to:

Sprawdzanie masz w konstruktorze przecież. Po co 2x to samo robić. Tą walidację też można do oddzielnej metody przenieść.

- database.php @instance() - To masz bez sensu bo:

będzie zawsze prawdą, $instance będzie instancją PDO a nie Database

- Controller_Auth

Nie można tego było przenieść po prostu do oddzielnej metody jako punkt obsługi formularza? Wtedy słałbyś form na tą metodę i sobie działał.

- Rozumiem że logowanie usera to tylko taki mały żarcik i że nie chciało Ci się nic w tym kierunku robić

- Nazwy klas typu: Controller_Base. Już chyba wiem skąd to Ci się wzięło. Skoro uczyłeś się na Kochana czy czymś innym, to tam są takie nazwy. Ale są dlatego że dzięki nim robiony jest autoload.
Tak, wystarczy zamienić _ na / i masz ścieżkę do klasy

-

Wywal to zanim ktoś to zobaczy

-

Prawda że czytelniej? I nie musisz potem całego dobrego kodu pakować w tego nieszczęsnego IF


Ogólnie moja uwaga jest taka. Staraj się stosować jak najmniej zagłębień kodu. czyli nie rób 10x IF w IF.
Staraj się układać tak warunki żeby kod był czytelniejszy. Nie sądzisz że skoro masz sprawdzanie czegoś od czego zależy czy dana metoda przejdzie czy nie to lepiej to umieścić jako pierwszy warunek i wysypać błąd niż pisać kolejne IF?

To tak na szybko zebrane do kupy. I na prawdę poczytaj o PSR-... Nie chcesz namespace? to nie stosuj. Nikt tego nie nakazuje.
Pozdrawiam i siema

PSR i namespace - tak jak napisali poprzednicy. To na prawdę nie bili a ułatwi życie.

Nie będę powtarzał tego, co napisali poprzednicy więc dodam tylko kilka rzeczy od siebie.

1. Masz mnóstwo śmieciowych komentarzy. Na przykład:

[PHP] pobierz, plaintext 
// --- Start sesji uzytkownika ---
    Session::instance()->start();
[PHP] pobierz, plaintext 

Bez komentarza dokładnie wiadomo co się dzieje. Nie musisz tego dodawać w komentarzu. Generalinie unikaj inline comments. Jeśli kod nie jest czytelny i musisz go komentować to znak, żeby zmienić kod (a nie dodawać komentarz). Dodanie komentarza w niczym nie pomoże,

2. Zapoznaj się z http://www.phpdoc.org/
Szczególnie z takimi rzeczami jak @param, @return, @throws. Bardzo ułatwia życie.

3. Exception404
Masz header na poziomie wyjątku. Można by się spierać, czy aby na pewno to dobry pomysł. Fakt, że rzucasz wyjątkiem 404 to nie znak, że natychmiast musi pójść header. W twoim przypadku nawet parent::__construct() nie ma tam racji bytu, bo obiekt nigdy nie będzie utworzony.
Wyjątek nie powinien mieć takiego zachowania. To aplikacja powinna decydować co ma zrobić z wyjątkiem a nie sam wyjątek. O to właśnie chodzi w wyjątkach.

4. html.php
Zapoznaj się z czymś takim jak trait. To może być lepsze rozwiązanie niż to co masz teraz.

5. Staraj się ograniczyć używanie typów prymitywnych. Przykład:

[PHP] pobierz, plaintext 
public static function add($name, $expression, array $defaults = NULL)
    {
        self::$routes[$name] = array(
            'expression' => $expression,
            'defaults' => $defaults
        );
    }
[PHP] pobierz, plaintext 

Nie masz kontroli na tym co i gdzie przekazujesz. Nie tylko moge przekazać co chcę, gdzie chcę i jak chcę to w sumie nie mam pojecia co ma być gdzie i w jakiej postaci. Generalnie bez dogłębnej analizy kodu nie mam pojęcia co się dzieje. A i mogę narobić burdelu bo nikt nigdzie niczego nie sprawdza. Generalnie tto jest proszenie się o problemy.


Tak jak napisali poprzednicy: OOP a używanie obiektów to nie to samo. Ty używasz obiektów a od tego jeszcze daleka droga do OOP.

Nope. Przydatne komentarze są przydatne. Ale komentuje się bloki kodu, a nie pojedyncze linijki. No chyba, że używamy jakiegoś hacka... ale takie rzeczy to najczęściej w cssie.

A sytuacji, gdzie komentarz jest przydatny jest jak na lekarstwo. Tak na szybko to jedyne co mi przychodzi do głowy, to praca z zewnętrznymi bibliotekami, które na przykład indeksują od 1 a nie od 0 i we własnym kodzie masz na przykład "$index + 1". Ale to są wyjątki. We własnym kodzie poza phpdoc metod (co wchodzi, co wychodzi i czy wyrzuca wyjątki) komentarz jest zawsze zbędny. A jeśli widzisz, że musisz dodać komentarz to zazwyczaj znaczy, że coś z kodem jest nie tak.
Wiem, że nazwenictwo metod i klas jest trudne. Tak samo jak rozbijanie mednej większej metody na mniejsze o sensownych nazwach. Ale to właśnie pozwala na kompletne uniknięcie komentarzy. Do tego trzymanie się SOLID (ze szczególnym naciskiem na S i D) bo wtedy jak na dłoni widać co jest gdzie i po co.

PS. To zna chyba każdy programista bez względu na język programowania, ale to bardzo dobrze oddaje wagę prawidłowego nazwenictwa:
"There are two hard things in computer science. Cache invalidation, naming things and off-by-one errors."

Nie. Komentarz traktuję jako streszczenie kodu, a nie jego wyjaśnienie. Szybciej mi przeczytać jedną linijkę komentarza, niż 10 linijek kodu. Warto też dodać, że początkujące osoby powinny pisać komentarze częściej, gdyż ich kod często prócz streszczenia, potrzebuje też wyjaśnienia.

Zaledwie nie sugerowałbym, by unikał komentarzy (gdyż to znaczy, że całkowicie z nich zrezygnuje). Z czasem zauważy, które jego komentarze są potrzebne, a które nie.

Całej reszty nie komentuję, gdyż z resztą się w pełni zgadzam.

Tuminure, po to jest phpdoc, generujesz sobie go i masz streszczenie, polecam lekturę czysty kod

do repo commity tez pisz po angielsku jest to język miedzynarodowy, ktoś kiedyś może trafić na twój kod i zechce zrobić forka i nawet jakiegoś pr, to nie będzie sobie polskiego tłumaczył żeby to prześledzić

Nie zgodzę się. Łatwiej przyczytać nazwę metody (i klasy), jej parametry wejściowe i wyjściowe niż jakikolwiek komentarz. Jeśli po tym dalej nie wiesz co się dzieje, to śmiem twierdzić, że problem leży w kodzie a nie w braku komentarza.



Tutaj też się nie zgodzę. Raz wyuczonych złych nawyków bardzo trudno się pozbyć. Dlatego dalej uważam, że od samego początku powinno się traktować komentarze wyłącznie jako ostateczność i to tylko w sytuacji, jeśli kodujemy na granicy. We własnym kodzie możesz napisał naście tysięcy linijek kodu i nie porzebować ani jednej linijki komentarza.

Poza tym komentarze mają jeszcze jeden, o wielei większy problem. Często opisuję jakiąś ogólną koncepcję czy logikę tego, co się dzieje w środku. Potem kod jest poddawany ciągłej re-faktoryzacji i zmianą. Mało kto pamięta o komentarzach. Nie mówiąc o sytuacjach, gdzie komentarz mówi coś w stylu "tu robię tak bo metoda X w klasie Y działa tak i tak". Potem ktoś zmienia klasę Y i nawet nie wie, że gdzieś tam w szerokim świecie jest już nieaktualny komentarz.

Absolutnie nie twierdzę, że komentarze są kompletnie zbędne. Ale powinno się ich używać tylko w na prawdę uzasadnionych sytuacjach.

A patrząc na kod autora wiele komentarzy mówi dokładnie to damo to linijka pod komentarzem. To jest tylko niepotrzebne śmiecenie kodu i na pewno nie przyniesie niczego dobrego.

PS. A do wyjaśnienia kodu służą testy jednostkowe. Wystarczy na nie popatrzeć i już wiesz co i jak działa i czego się spodziewać po kodzie.

Dziękuję Wam wszystkim Dzięki Waszym uwagom w 2 dni nauczyłem się używania przestrzeni nazw, composera, konwencji pisania, nazewnictwa W PSR-3 zauważyłem również fajny komponent do logowania errorów, warning, itd., może przyda się W PSR-7 również zrobili część frameworka - klasy Request, Response i HTTP.

Resztę Waszych uwag muszę jeszcze dokładnie prześledzić, i spróbować je zastosować


Jeśli ktoś jeszcze miałby jakieś sugestie, feedback to chętnie