• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Prośba o ocenę strony i kodu V2

VPS Starter Arubacloud
+2 głosów
993 wizyt
pytanie zadane 14 sierpnia 2016 w PHP przez GaCeL Dyskutant (7,500 p.)
edycja 14 sierpnia 2016 przez GaCeL

Cześć, jakoś nie dawno temu oddałem wam do oceny mój pierwszy większy projekt w PHP, przeczytałem wszystkie oceny, i wziąłem sobie do serca większość z nich, od razu postanowiłem że muszę wybrać jakiś framework, wybór padł na Symfony i wydaje mi się że dobrze wybrałem, chociaż nawet jeszcze nie do końca wiedziałem wszystko o PHP(i dalej dużo nie wiem), i nie wiem czy mi to wyszło na dobrze, mogę nawet powiedzieć że wiedziałem bardzo mało(bo nie dokończyłem nawet całego kursu PHP pana Mirosława). Ale małymi kroczkami jakoś uczyłem się właśnie tego frameworka, pracując właśnie przy omawianym projekcie.

Zrefaktoryzowałem stary projekt, mowa o tym http://forum.pasja-informatyki.pl/126751/prosba-o-ocene-strony-i-kodu i sami oceńcie jak to mi wyszło, wiem że nie jest idealnie ale starałem się jak tylko mogłem.

Testowe konto:
Login: boss Hasło: boss123
Zaproszenia potrzebne do rejestracji:

Link do source: https://github.com/gacel112/atccargo
Link do projektu: http://gacel112.webd.pl/

2 odpowiedzi

+11 głosów
odpowiedź 19 sierpnia 2016 przez matiit Użytkownik (560 p.)

Hej, to moja pierwsza wizyta tutaj. 
Nie znam dokładnie zasad tutaj panujących, ale postaram się po prostu zrobić Code Review jaki normalnie robię w pracy. 

 

Pierwszą rzeczą do której można by się przyczepić to GIT.

Większość pracy jest zrobione w pierwszym commicie. Sprawia to, że jest trudno zrobić code review 'po kawałku'. Według mnie, byłoby dużo lepiej gdyby każdy krok jak: 

  • Instalacja symfony
  • Konfiguracja
  • Utworzenie Bundla (akurat w tym przypadku nie jest to ważne)
  • Dodanie nowej zależności
  • Dodanie nowej funkcjonalności

były w osobnych commitach.

To jest pierwsza rzecz. 

Przechodząc do kodu. 
Na codzień nie koduję w symfony, i symfony znam dość słabo. Używam poszczególnych komponentów, ale ze nie miałem nigdy do czynienia z żadnym skomplikowanym projektem w Symfony. Wiele praktyk jest oczywiście podobnych "across frameworks", więc postaram się skomentować nawet na temat projektu :)

Masz tylko jeden Bundle (AppBundle). Przy projekcie tej wielkości nie ma to znaczenia, ale jeśli postanowiłbyś go rozbudowywać, na pewno zacznie to ciążyć i dojdziesz do wniosku, że warto byłoby wyciągnąć niektóre rzeczy do oddzielnych bundlów. Po prostu dla przejrzystości.

Dalej. Nazwy metod. 


Pierwszy z brzegu przykład:

DefaultController::selectAction

Sygnatura tej metody w ogóle nie mówi mi co ten kontroler robi. Co jest wybierane? Czy w ogóle coś jest wybierane? Czemu to się tak nazywa? Chociażby jakiś komentarz by się przydał, żeby było wiadomo o co chodzi.

Jest więcej miejsc gdzie można było nazwać kontrolery inaczej (kontroler dla mnie to "akcja", nie "klasa".

Ok dalej.

Metody same w sobie.

Przykład:

    public function addAction(Request $request)
    {
        $invitation = new Invitation();
        $form = $this->createForm(InvitationType::class, $invitation);
        $form->handleRequest($request);
        if ($form->isSubmitted() && $form->isValid()) {
            $invitation->setCode(rand(10000, 99999));
            $em = $this->getDoctrine()->getManager();
            $em->persist($invitation);
            $em->flush();
            $message = \Swift_Message::newInstance()
                ->setSubject($this->getParameter('invitation_email_subject'))
                ->setFrom($this->getParameter('invitation_email_from'))
                ->setTo($invitation->getEmail())
                ->setBody(
                    $this->renderView(
                        '@App/invitation/email.html.twig',
                        array(
                            'position' => $invitation->getRoles(),
                            'email'    => $invitation->getEmail(),
                            'code'     => $invitation->getCode()
                        )
                    ),
                    'text/html'
                )
            ;
            $this->get('mailer')->send($message);
            $this->addFlash(
                'notice',
                'Invitation has been sent!'
            );
            return $this->redirectToRoute('app_invitation_add');
        }
        return $this->render('@App/invitation/add.html.twig', array(
            'form' => $form->createView()
        ));

Tutaj nie mam problemu z nazwą. 
Problem mam z długością tej metody. 

~40 lini. Nie ma tragedii, ale dałoby się zrobić to fajniej wg mnie. Chociażby:

  • Wysyłanie e-maila - conajmniej powinno być to w prywatnej metodzie, a najlepiej to w ogóle w oddzielnej klasie. W dłuższej perspektywie pewnie jakiś system typu Rabbitmq powinien się tym zajmować (możesz mieć handlery do tego w PHP oczywiście).
     
  • $invitation->sendCode - na razie jest to proste wywołanie rand, w przyszłości zmieni się to napewno i nie będzie tak fajnie ustawiać tego w miejscu. Wydzielenie tego byłoby wskazane.

Bliźniacze "błędy" są w reszcie kontrolerów.

Błędy w cudzysłowiu, bo nie są to stricte błędy  - po prostu można by zrobić to czyściej.

TwigExtension który dodałeś. 
Nie wiem na ile ta praktyka jest stosowana na codzień przez developerów piszących w Symfony, ale mi się nie podoba. 
Za dużo odniesień do bazy danych poprzez Doctrine z poziomu rozszerzenia Twiga.

Oznacza to, że nie będzie łatwo w przyszłości zmienić ani Twiga, ani Doctrine.

Wg mnie możnaby to zrobić lepiej chociażby poprzez dodanie 1 abstrakcji. Repozytoriów (ale nie Doctrine Repository) - które w tym przypadku implementowałyby tylko dostęp przez Doctrine - bo nic innego w tym momencie nie potrzeba.

Plusem takiego rozwiązania byłaby szybka możliwość zmiany Doctrine w razie potrzeby - wystarczyłoby dopisać "driver" - po prostu implementacje tego repo dla "innego Doctrine".

 

Brak docblocków. Kod wygląda na PHP 5.4+, nie widzę użycia featureów z php7 - docblocki przydałyby się dla metod które przyjmują jako argumenty prymitywne typy jak integer. 
Wg mnie zwiększyłoby to czytelność kodu - a na pewno lepiej pokazywałoby intencje. 

 

Dalej. Znowu nazwy metod.
Tym razem w repozytorium "TransportRepository". Jest tam metoda, którą nazwałeś: "findAllActiveBy".

Co to nam mówi? Że coś będzie szukane. Że będziemy szukać wszystkich aktywnych. "By" - no ale "By what?".

"findAllActiveByUserId" wg mnie tutaj byłoby dużo lepszą nazwą. Trochę dłuższa - prawda - ale o ile więcej nam mówi, zgodzisz się?

"findAllNotActive" "findAllInactive"
 

"findFiveActiveBy" To samo co findAllActiveBy. 
 

Poza tym chyba lepiej byłoby dać "Five" jako parameter.

Dalej. Users Repository. 
Metoda "findDrivers" 
W sqlu robisz różne magiczne rzeczy, które nijak się mają do nazwy metody. 
Linijka z sqlem jest za długa - lepiej to rozbić na kilka.
Poza tym co zwraca ta metoda? Driverów? Masz taką encję? 
Czy zwraca Userów którzy są Driverami? Może warto byłoby zrobić klase Driver dziedziczącą po User?

 

Dalej. 
Kilka "braków" w anglielskim.
"madedTransports" - no nie :) 
Oczywiście nie czepiam się - tylko zwracam uwagę. I tak wielki plus ze kod jest po angielski, a nie po Polsku. Z czasem po prostu poprawisz angielski i takich błędów się będziesz wyzbywał.


Dalej.

"UsersRepository::getFuel($id)" - Znowu słaba nazwa - jakie paliwo? Domyślam się że tego użytjkownika. Ale czy wynik jest w litrach? Czy w galonach? A może to po prostu boolean mowiacy czy jest jakiekolwiek paliwo "u użytkownika?" a może "dla użytkownika?" a może "do użytkownika"? 
No nic nie wiadomo po prostu. Chociaż jakiś komentarz by się przydał. I lepsza nazwa. A może i lepsze miejsce.

 

Dalej - pewnie byłaby to po prostu kompilacja powyższego. 

Mam nadzieję, że pomogłem.

 

 

komentarz 19 sierpnia 2016 przez GaCeL Dyskutant (7,500 p.)
edycja 19 sierpnia 2016 przez GaCeL
Dziękuje za Code review, czekam na kolejne. Zastosuje się do wszystkich zawartych tu porad :)

Jeżeli zmienna klasy zawiera "datetime" to jaki @var ustawić w docblocku?
1
komentarz 19 sierpnia 2016 przez matiit Użytkownik (560 p.)
Nie musisz się stosować do wszystkich porad bo pewnie dla tak małego projektu to "za dużo". Polecałbym sięgnięcie po jaką lekturę typu Clean Code.
1
komentarz 19 sierpnia 2016 przez efiku Szeryf (75,160 p.)
Ja polecam od siebie poczytać o serwisach w Symfony i to je wołać w kontrolerach, bo jest tam zdecydowanie za dużo kodu. Tu aż się prosi aby podpiąć to wysyłanie maili jako serwis.

http://symfony.com/doc/current/service_container.html

Dobry CR, to teraz pasuje tylko wrócić do kodu i robić refactor.

+ TDD
komentarz 19 sierpnia 2016 przez efiku Szeryf (75,160 p.)

TwigExtension który dodałeś.  
Nie wiem na ile ta praktyka jest stosowana na codzień przez developerów piszących w Symfony, ale mi się nie podoba.  
Za dużo odniesień do bazy danych poprzez Doctrine z poziomu rozszerzenia Twiga.

Rozszerzenie Twiga to po prostu zazwyczaj kolejna funkcja do obrabiania tekstu, ale na pewno nikt z programistów Syfmony nie umieszcza tam odwołań do Doctrine :D) Po prostu, przekazujemy do szablonu odpowiednie dane w tablicy ( $this->render("plik", [dane]) , następnie w plikach szablonów stosujemy przez nas funkcje np.

{{ user.name|capitalize }}

Ja bym dodał jeszcze od siebie, abyś GaCeL zerknął na klasę Controller po której dziedziczysz kontrolery. 

http://symfony.com/doc/current/best_practices/controllers.html

komentarz 19 sierpnia 2016 przez GaCeL Dyskutant (7,500 p.)
Mówisz że w Twig Extension nie powinno się odwoływać do Doctrine, jak najlepiej zrobić właśnie te zmienne(statystyki użytkownika) które były by dostępne w każdym szablonie(globalnie)
Jeszcze muszę się ciebie spytać co to ten skrót CR?

@ParamConverter?
komentarz 20 sierpnia 2016 przez HaKIM Szeryf (87,590 p.)
Jak na moje CR to CodeReview. ;)

Który, nawiasem mówiąc, został zrobiony dobrze. Więcej takich ludzi na forum, poproszę! :)
komentarz 20 sierpnia 2016 przez matiit Użytkownik (560 p.)
Dziękuję,

Chociaż przyznam, że trochę się niewygodnie to robi w ten sposób - oglądanie kodu na githubie, a potem pisanie komentarzy tutaj na forum.

Lepiej byłoby, żeby kod był podany jako Pull Request gdzie można komentować na diffie.
–1 głos
odpowiedź 20 sierpnia 2016 przez Artur Kaniowski Nowicjusz (140 p.)

Sorry za offtop, ale chciałem pogratulować. Skopałeś dzisiaj tyłek Google i Facebookowi.

komentarz 20 sierpnia 2016 przez efiku Szeryf (75,160 p.)
edycja 20 sierpnia 2016 przez efiku

a te fejkowe konta na fallowersach to pewnie błąd skryptu ;)

Byle by gwiazdki nabić i fallowersów mieć.
Ale to nie tędy droga ;)

cenzura  oszuści.

2
komentarz 20 sierpnia 2016 przez SyntaxError Pasjonat (17,170 p.)
'Oszukali mnie, banda złodziei..'

Podobne pytania

0 głosów
1 odpowiedź 174 wizyt
pytanie zadane 24 lutego 2021 w JavaScript przez malvator Użytkownik (720 p.)
0 głosów
1 odpowiedź 214 wizyt
pytanie zadane 26 lipca 2020 w JavaScript przez rob Bywalec (2,440 p.)
+1 głos
1 odpowiedź 160 wizyt
pytanie zadane 20 lipca 2020 w JavaScript przez rob Bywalec (2,440 p.)

92,841 zapytań

141,781 odpowiedzi

320,857 komentarzy

62,173 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Wprowadzenie do ITsec, tom 2

Można już zamawiać tom 2 książki "Wprowadzenie do bezpieczeństwa IT" - będzie to około 650 stron wiedzy o ITsec (17 rozdziałów, 14 autorów, kolorowy druk).

Planowana premiera: 30.09.2024, zaś planowana wysyłka nastąpi w drugim tygodniu października 2024.

Warto preorderować, tym bardziej, iż mamy dla Was kod: pasja (użyjcie go w koszyku), dzięki któremu uzyskamy dodatkowe 15% zniżki! Dziękujemy zaprzyjaźnionej ekipie Sekuraka za kod dla naszej Społeczności!

...