URL shortener - Symfony 4. Opcje

[smk]

Witam.
Prosiłbym Was o code review aplikacji do skracania adresów (IMG:style_emoticons/default/smile.gif) Czym więcej feedbacku, tym lepiej!
https://github.com/selfmadeking/URL-shortener-Symfony-4

[athabus]

Kurcze, aż szkoda żeby taki fajny temat się zmarnował, więc choć się nie znam, to pierwszy rzucę kamieniem - może bardziej doświadczeni koledzy się przyłączą ;-)

Po pierwsze bardzo nie podoba mi się brak typowania zmiennych i pól w klasie.

[PHP] pobierz, plaintext 
private $urlGeneratorService;
    private $em;
 
    public function __construct(UrlGeneratorService $urlGeneratorService, EntityManagerInterface $em)
    {
        $this->urlGeneratorService = $urlGeneratorService;
        $this->em = $em;
    }
[PHP] pobierz, plaintext 

Dlaczego pola klasy nie mają docka z typami? Przecież korzystasz z phpstorma, a on potrafi wygenerować - stajesz na parametrach konstruktora i klikasz alt+enter....

Większość funkcji nie ma typowania zmiennych na wejściu, ale jednocześnie w niektórych miejscach masz typowanie na wyjściu, czyli używasz php 7.1+ - brakuje konsekwencji. W każdym razie mz w jakiś sposób trzeba parametry opisać - albo phpdock albo typowanie.

Zaciekawiła mnie funkcja, którą wezmę jako przykład na warsztat.

[PHP] pobierz, plaintext 
public function userIsAuthorOfList($list)
    {
        $urls = $list->getListOfUrls();
        $userId = $this->security->getUser();
        if (isset($userId)) {
            $userId = $userId->getId();
        }
        $userIdOfUrl = $urls->first()->getUserId();
        if (isset($userIdOfUrl)) {
            $userIdOfUrl = $userIdOfUrl->getId();
        }
 
        return (bool) $userIdOfUrl == $userId;
 
    }
[PHP] pobierz, plaintext 

$list - nie jest typowana na wejsciu. Co to jest - jakiś obiekt, zmienna skalarna, tablica? Lista czego?
Dalej w tej samej funkcji masz:

[PHP] pobierz, plaintext 
$userId = $this->security->getUser();
        if (isset($userId)) {
            $userId = $userId->getId();
        }
 
[PHP] pobierz, plaintext 

Czyli co zmienna $userId jest tak na prawdę obiektem User, ale za chwilę zamieniasz ją na skalarne Id? Zupełnie bez sensu bo kompletnie psuje czytelność kodu.

W tej funkcj imasz coś takiego:

[PHP] pobierz, plaintext 
$userIdOfUrl = $urls->first()->getUserId();
[PHP] pobierz, plaintext 

Co tak na prawdę jest $userIdOfUrl = $list->getListOfUrls()->first()->getUserId();
Poczytaj o Lod (Law of Demeter) - raczej powinno być coś w stylu $list->getUser() lub $list->getUserId(); Zresztą nie sprawdzałem, ale mogę się założyć, że w innych częściach systemu robisz coś podobnego, aby sprawdzić użytkownika listy = duplika kodu, czyli tym bardziej warto to zrefaktoryzować.

Po co rzutujesz wartość bool do bool w tym miejscu? To chyba zbędne.

[PHP] pobierz, plaintext 
return (bool) $userIdOfUrl == $userId;
[PHP] pobierz, plaintext 

Ogólnie ta funkcja to idealny kandydat do refactoringu na wielu płaszczyznach. Powinno się ją sprowadzić do 2-3 linijek.

Kilka luźnych uwag:
- servisy nie powinny raczej zapisywać niczego do flash - to rola kontrolera - serwis może być wykorzystywany w inny sposób (np. api/cli itp).
- logując wyjątek logujesz tylko sam message - mało to przydatne przy debugowaniu lepjej zrobić $logger->error((string)$e)
- w symfony if ($form->isSubmited() && $form->isValid()) możesz zastąpić if ($form->isValid()) - bo metoda isValid sprawdza między innymi czy form jest wysłany.


To takie moje 0,05pln żaby rozpocząć temat. Niestety nie mam teraz czasu na wgryzienie się w Twój system i pisanie czegoś więcej o logice itp, dlatego tylko kilka uwag co do samego sposobu kodowania. Dodam, że raczej mistrzem kodowania nie jestem, więc traktuj moje uwagi jako wymianę zdań.