Prośba o ocenę kodu

Question

Hej, jestem samoukiem i raczkuję z php od paru miesięcy, napisałem w ostatnim czasie małą apke - logowanie, rejestracja, zmiana hasła z poziomu zalogowanego użytkownika oraz zawieszenie konta. Po zalogowaniu jest dostępna strona z profilem użytkownika w pasku menu.

Modal z ustawieniami dla użytkownika zrobiłem w pewnej części w JQ oraz zmiana hasła przy pomocy ajax:)

napisałem własny MVC w php starając się w miarę trzymać zasad PSR i OPP,

dodam że przyjmę na klate słowa krytyki:) wiem, że jestem początkującym programistą i pewnie popełniłem dużo błędów..

Proszę o ocenę kodu i ewentualnę wskazanie jakichś rozwiązań. Dołączam kod na githubie i stronę do obejrzenia live (trzeba się na niej zarejestrować i zalogować żeby zobaczyć jak wygląda)

http://testowastrona.stronazen.pl/mvc4

https://github.com/Brair/jetpack-v2

Czy taki kod można pokazać składając cv? czy jest to za mało by cokolwiek stwierdzić dla rekrutera. Chciałbym szukać pracy na stanowisko junior php dev^^

Answer 1

• Jaka logika stoi za podziałem na src i app? Czemu w src są kontrolery typowe dla danej aplikacji?

• https://github.com/Brair/jetpack-v2/blob/master/tests/exampleTest.php – wartałoby jednak napisać jakieś testy.

• https://github.com/Brair/jetpack-v2/blob/master/public/desktop.ini – po co to?

• https://github.com/Brair/jetpack-v2/blob/master/index.php – jaki cel ma ten plik?

• https://github.com/Brair/jetpack-v2/blob/master/config/config.php – mam nadzieję, że nie jest to Twój faktycznie używany config. Na GH powinien się znajdować szablon tego pliku. Jeszcze lepiej: poczytaj o przechowywaniu konfiguracji w env.

• https://github.com/Brair/jetpack-v2/tree/master/vendor – po co, skoro dołączasz composer.json pozwalający na doinstalowanie wszystkiego.

• https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthUser.php#L3 – namespace niekoniecznie zgadza się z wielkością liter w ścieżce.

• https://github.com/Brair/jetpack-v2/tree/master/app – models i Models – jaki jest sens tego mylącego podziału? Inna rzecz, że na Windowsie czy macOS-ie całość się rozpadnie, bo tam wielkość liter nie ma znaczenia.

• https://github.com/Brair/jetpack-v2/blob/master/app/Models/Authentication/Password/Password.php – spójrz, jak dużo tutaj powtarzalnych części (zwłaszcza kodu do obsługi wyjątków). Aż się prosi o stworzenie osobnej klasy do obsługi bazy danych.

• Ogólnie ta klasa Password wydaje się za duża. Hashowanie wyciągnąłbym do osobnego serwisu, a walidację hasła do walidatora.

• Hasła na przyszłość: repozytorium, encja, Value Object.

• https://github.com/Brair/jetpack-v2/blob/master/app/Models/Authentication/Password/PasswordChange.php – to powinien być serwis, nie osobny model.

• https://github.com/Brair/jetpack-v2/blob/master/app/Models/Authentication/Register.php – tutaj znów: wyciągnięcie walidacji do walidatora a sam proces rejestracji do serwisu usprawni cały kod. No i modelem powinien być raczej User a nie Register.

• W sumie tu nigdzie nie ma pliku SQL ze strukturą bazy.

• https://github.com/Brair/jetpack-v2/blob/master/app/models/DB/DataBase.php#L17-L23 – tak po prawdzie zamiast echo (i to jeszcze bez die) lepiej byłoby rzucić własnym wyjątiem, który byłby przechwytywany przez core frameworka (np. przy pomocy exception handlera) i używany do wygenerowania sensownej strony błędu 500.

• https://github.com/Brair/jetpack-v2/blob/master/src/BaseController.php – czemu nazwa pliku nie zgadza się z nazwą klasy? Czemu nie jest to interfejs?

• https://github.com/Brair/jetpack-v2/blob/master/src/View.php#L20 – klasa View nie powinna mieć wiedzy o istnieniu sesji.

• https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L30-L41 – czemu autoloader tego nie robi?

• Ogólnie przy routingu widziałbym zastosowanie REST, a nie naiwne parsowanie adresu. Zasada kontroler-metoda-parametry jest strasznie sztywna i prowadzi do powstawania bezsensownych adresów.

• No i brakuje mi obiektów Request i Response.

• https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LoginController.php#L19 – jaki sens ma to strip_tags?

• https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LoginController.php#L16 – tak naprawdę chcesz sprawdzić, czy dane przyszły POST-em, więc raczej $_SERVER['REQUEST_METHOD'] === 'POST'.

• https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L79-L80 – nie sądzisz, że to aż dziwnie wygląda?

• https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L80-L82 – po co te kombinacje alpejskie skoro metoda createUser mogła zwrócić id ostatnio wstawionego rekordu?

• https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L80-L82 – po co Ci osobny model tylko i wyłącznie do ustawiania daty urodzin?

• https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthPage.php#L17-L24 – to można trzasnąć jednym if:

    if ( !$_SESSION['user'] || $_SESSION['user']->role !== 'admin'){
        return header('Location: 404');
    }

  No i co to ma wspólnego z modelami?

 

Comments

dzięki za odpowiedź juz poprawiam błędy:)

Answer 2

https://github.com/Brair/jetpack-v2/blob/master/tests/exampleTest.php - nie widzę zachowania PSR

https://github.com/Brair/jetpack-v2/blob/master/src/View.php#L6 - type-hint dla returnu i argumentów

https://github.com/Brair/jetpack-v2/blob/master/src/View.php#L20 - ścisłe porównanie ===, brak PSR (spacja pomiędzy ifem a klamrą)

https://github.com/Brair/jetpack-v2/blob/master/src/View.php#L26 - jw.

https://github.com/Brair/jetpack-v2/blob/master/src/View.php#L30 - pusty else

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L10 - brak return-hinta, chyba miało być parseUrl

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L13 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L18 - przeniósłbym na samą górę pliku

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L30 - brak PSR, w ifie nie powinno być żadnych wywołań funkcji itp. (przypisz to do innej zmiennej i wtedy sprawdzaj)

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L36 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L40 - unused code

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L44 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L46 - jw.

https://github.com/Brair/jetpack-v2/blob/master/src/Routing.php#L60 - unused code

https://github.com/Brair/jetpack-v2/blob/master/src/BaseController.php#L5 - to powinien być komentarz nad klasą

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/HomeController.php#L13 - niepotrzebny komentarz

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LoginController.php#L22 - ===

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LoginController.php#L26 - na początku pobrałbym to w jakąś zmienną i potem tylko się do niej odwoływał

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LoginController.php#L33 - to chyba klamra kończąca ifa. Dlaczego tam jest średnik?

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/LogoutController.php#L18 - niepotrzebna array

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/MailController.php#L12 - po co ten komentarz?

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/MailController.php#L11 - raczej powinno zwracać widok

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L21 - zamień # na //

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L34 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L48 - zbyt długi if

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L45 - nazwa funkcji mało mówi o funkcjonalności, PSR-2 (camelCase)

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L57 -- PSR, ===

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L63 ===

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L81 - te wszystkie echa powinny być w zmiennej, potem przekazać do widoku a widok powinien to wyświetlić

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L92 - PSR-2 (camelCase)

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L94 - PSR-2

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L103 - jw

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L109 - jw

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L117 - jw

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/ProfileController.php#L125 - zamień na use i potem tylko User()

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L27 - ???

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L38 - ???

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L97 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/RegisterController.php#L99 - jw

https://github.com/Brair/jetpack-v2/blob/master/src/Controller/_404Controller.php#L12 - unused code

https://github.com/Brair/jetpack-v2/blob/master/public/index.php#L4 - unused code

https://github.com/Brair/jetpack-v2/blob/master/app/models/User/User.php#L11 - type hinty

https://github.com/Brair/jetpack-v2/blob/master/app/models/User/User.php#L14 - //

https://github.com/Brair/jetpack-v2/blob/master/app/models/User/User.php#L21 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/app/models/User/User.php#L37 - type-hinty

https://github.com/Brair/jetpack-v2/blob/master/app/models/User/User.php#L63 - jw.

https://github.com/Brair/jetpack-v2/blob/master/app/models/DB/DataBase.php#L27 - brak deklaracji widoczności

https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthPage.php#L6 - brak PSR

https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthPage.php#L21 - ???

https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthPage.php#L20 - ===

https://github.com/Brair/jetpack-v2/blob/master/app/models/Authentication/AuthUser.php#L10 - brak PSR w całym pliku (po co te wcięcia ? )

 

Comments

Skoro piszesz tylko o syntaxie to lepiej było wkleić out z PHPCSa. 

---

@Mariusz08, dzięki za odpowiedź juz poprawiam błędy:)

---

@Ehlert, już włączyłęm phpcs Dzięki

Answer 3

Ja tam za dużo w PHP nie siedzę, ale jak zajrzałem do kodu, to mam wrażenie, że można pozmniejszać niektóre metody, bo mają zdecydowanie za dużo odpowiedzialności. Gdzieś tam nawet widziałem potwornego zagnieżdżonego ifa, w którym bardzo łatwo można się pogubić.

Comments

dzięki za odpowiedź. Postaram się odnaleźć i poprawić to:)

czy ktoś jeszcze może rzucić okiem:)?