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.
