Prośba o code review - PHP REST

pytanie zadane 29 lipca 2018 w PHP przez User007 Bywalec (2,400 p.)

Witam.

Proszę o ocenę kodu małej aplikacji RESTowej.

Głównym celem aplikacji jest zapisywanie danych statystycznych (dane użytkownika, statystyki poziomów) z gry na platformę Android.

Główny rdzeń aplikacji jest ukończony. Do ukończenia pozostało jeszcze napisanie paru zapytań do bazy danych itp. Ale to szczegóły.

Jedyna BARDZO ważna rzecz która została do zrobienia to system uwierzytelniania zapytań z urządzeń mobilnych.

Ale może mój zamysł jak to ma działać opiszę w innym pytaniu aby tu nie zbaczać od głównego tematu. W każdym bądź razie mam parę pytań jak to najlepiej zrobić.

Co do apki tytułem wstępu to, podzielona ona jest na dwa segmenty:

/core - katalog gdzie jest trzymany silnik aplikacji,

/http - katalog gdzie można dodawać obsługę nowych zapytań url

Link do projektu: https://github.com/kapdom/Planetoid-REST

3 odpowiedzi

odpowiedź 29 lipca 2018 przez Comandeer Guru (600,810 p.)

Skoro kontroler zawiera żądanie, to czemu nie zawiera też odpowiedzi?
Czemu ¾ kodu jest oparte na static? To sprawia, że to de facto nie jest kod OOP. Statyczne metody zachowują się przecież jak normalne funkcje, bez trzymania stanu.
Jak tak patrzę w kod modelu, to widzę, że większość tych metod ma prawie to samo ciało. Ergo: wypadałoby to zrefaktorować.
No i czemu modele są wewnątrz przestrzeni http? To nielogiczne.
https://github.com/kapdom/Planetoid-REST/blob/master/app/http/models/Result.php – to raczej encja a nie model.
Czemu jest różnica między nazwą katalogu (validator) a nazwą przestrzeni nazw (Validator)? Jeśli przestrzeń nazw jest wykorzystywana do wczytywania klas, to na systemach case-sensitive (np. Linux) całość może nie działać.
https://github.com/kapdom/Planetoid-REST/blob/master/app/core/bootstrap/Bootstrap.php – po co to jest klasą?
https://github.com/kapdom/Planetoid-REST/blob/master/app/core/config/config.txt – a to nie powinno mieć rozszerzenia .ini? Inna rzecz: https://12factor.net/config
Ogólnie to nie robiłbym własnego frameworka, ale wziąłbym jakieś maleństwo pokroju Slima czy Symfony 4 (w tej domyślnej, małej wersji) i na tym po prostu dopisał kontrolery.

komentarz 29 lipca 2018 przez User007 Bywalec (2,400 p.)

Dziękuję za odpowiedzi. Przyjmuję wszystko do wiadomości :) z małymi pytaniami.

Zaczynając od końca.

Framework własny zrobiłem tylko po to aby się lepiej PHP nauczyć. I może żeby do portfolio dodać aby pokazać coś potencjalnemu pracodawcy. Jeśli moje rozumowanie jest błędne proszę napiszcie to. Chociaż w obecnym stanie to się to pewnie nie nadaje.

Config ma rozszerzenie .ini, tylko na github jest wysyłane txt.

W przypadku validatora i innych klas to zawsze przestrzenie nazw są z dużej litery a katalogi z małej. Ktoś mi kiedyś napisał że tak ma być.

Dlaczego uważasz że Result.php to encja a nie model?

A po co mam trzymać jakikolwiek stan w statycznych metodach? Przecież ma to tylko wykonać działanie i zwrócić wynik. Myślałem że metody i właściwości statyczne świetnie się do tego nadają. Możesz dokładniej wyjaśnić dlaczego to jest problemem?

P.S. Napiszcie proszę chociaż jedną rzecz którą dobrze zrobiłem, bo się załamię :).

komentarz 29 lipca 2018 przez Comandeer Guru (600,810 p.)

Jeśli moje rozumowanie jest błędne proszę napiszcie to.

Twoje rozumowanie jest błędne ;) Jasne, frameworki się fajnie pisze, ale żeby to napisać dobrze, to trzeba mieć naprawdę sporo wiedzy i doświadczenia – choćby po to, by analizować inne frameworki i wyciągać wnioski z ich architektury. Inaczej powstanie coś, czego sami będziemy się wstydzić, gdy tylko nasza wiedza i doświadczenie wzrosną.

Zresztą większość produktów opiera się na ogólnie znanych i szanowanych frameworkach pokroju Symfony czy Laravela. Czemu? Bo to są frameworki popularne, przetestowane w boju, opierające się często na najlepszych praktykach w branży. Po co wymyślać koło na nowo? Lepiej ten czas poświęcić na implementację logiki biznesowej. Z tego też względu bardziej ceni się (z punktu widzenia biznesu) umiejętność pracy w znanych frameworkach niż umiejętność pisania własnego. To drugie znajduje zastosowanie raczej w naprawdę dużych, korporacyjnych projektach, gdzie czas do wdrożenia nie jest kluczowym czynnikiem, a zasoby pieniężne po prostu są.

Config ma rozszerzenie .ini, tylko na github jest wysyłane txt.

A czemu?

W przypadku validatora i innych klas to zawsze przestrzenie nazw są z dużej litery a katalogi z małej. Ktoś mi kiedyś napisał że tak ma być.

Nie bardzo wiem, czemu miałoby tak być? Tego typu rzeczy naprawdę sprawiają problemy przy przenoszeniu aplikacji między różnymi systemami.

Dlaczego uważasz że Result.php to encja a nie model?

Bo model dla mnie to bardziej rozbudowane pojęcie, do którego wciska się wiele rzeczy (choćby właśnie pobieranie danych). Encja z kolei po prostu te dane reprezentuje.

A po co mam trzymać jakikolwiek stan w statycznych metodach?

Skoro nie masz stanu, to po co Ci klasy i obiekty? Tutaj jest zarzewie problemu: jeśli całość jest bezstanowa, to równie dobrze możesz to przepisać na zwykłe funkcje i dalej będzie działać. Tu nie chodzi o samą implementację, a o sposób myślenia o aplikacji.

Napiszcie proszę chociaż jedną rzecz którą dobrze zrobiłem, bo się załamię :)

Cóż, ja jestem okrutnym człowiekiem >:D. Ale mogę Ci na pocieszenie powiedzieć, że moje początki z PHP wyglądały bardzo podobnie. Niektóre rzeczy nabywa się po prostu wraz z doświadczeniem ;)

komentarz 29 lipca 2018 przez User007 Bywalec (2,400 p.)

Config ma rozszerzenie .ini, tylko na github jest wysyłane txt.

A czemu?

A no temu, aby na githuba nie wysłać swojej konfiguracji dostępu do bazy danych itp. A zamiast tego wysyłać jakby szablon konfiguracyjny. To też mi ktoś tak "doradził".

Skoro nie masz stanu, to po co Ci klasy i obiekty? Tutaj jest zarzewie problemu: jeśli całość jest bezstanowa, to równie dobrze możesz to przepisać na zwykłe funkcje i dalej będzie działać. Tu nie chodzi o samą implementację, a o sposób myślenia o aplikacji.

No racja, racja.

Nie bardzo wiem, czemu miałoby tak być? Tego typu rzeczy naprawdę sprawiają problemy przy przenoszeniu aplikacji między różnymi systemami.

Sprawdziłem teraz na szybko laravela i rzeczywiście jest tak jak napisałeś. Nie wiem kto mi to wcisnął że tak zrobiłem jak zrobiłem.

Dzięki za review, dam znać jak wprowadzę zmiany.

komentarz 29 lipca 2018 przez Comandeer Guru (600,810 p.)

A no temu, aby na githuba nie wysłać swojej konfiguracji dostępu do bazy danych itp. A zamiast tego wysyłać jakby szablon konfiguracyjny.

W sumie robi sensy, chociaż chyba bym pomyślał nad config.template.ini lub czymś podobnym i odpowiednim ignore w .gitignore dla rzeczywistego pliku z konfiguracją. Niemniej mimo wszystko polecałbym spojrzeć na to, co podlinkowałem, o trzymaniu konfiguracji w zmiennych środowiskowych.

komentarz 29 lipca 2018 przez User007 Bywalec (2,400 p.)

Tak zrobię.

komentarz 30 lipca 2018 przez CenterPL Pasjonat (19,070 p.)

Przyjęło się, że dla szablonowego configu robimy np config.ini.dist i wtedy problem z głowy. Ale co prawda, to prawda, orginał powinien być w .gitignore.