Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> URL shortener - Symfony 4.
smk
post 17.08.2018, 16:34:55
Post #1





Grupa: Zarejestrowani
Postów: 8
Pomógł: 0
Dołączył: 15.06.2018

Ostrzeżenie: (0%)
-----


Witam.
Prosiłbym Was o code review aplikacji do skracania adresów smile.gif Czym więcej feedbacku, tym lepiej!
https://github.com/selfmadeking/URL-shortener-Symfony-4
Go to the top of the page
+Quote Post
athabus
post 23.08.2018, 21:25:44
Post #2





Grupa: Zarejestrowani
Postów: 874
Pomógł: 44
Dołączył: 2.11.2005
Skąd: Poznań

Ostrzeżenie: (0%)
-----


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.

  1. private $urlGeneratorService;
  2. private $em;
  3.  
  4. public function __construct(UrlGeneratorService $urlGeneratorService, EntityManagerInterface $em)
  5. {
  6. $this->urlGeneratorService = $urlGeneratorService;
  7. $this->em = $em;
  8. }


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.
  1. public function userIsAuthorOfList($list)
  2. {
  3. $urls = $list->getListOfUrls();
  4. $userId = $this->security->getUser();
  5. if (isset($userId)) {
  6. $userId = $userId->getId();
  7. }
  8. $userIdOfUrl = $urls->first()->getUserId();
  9. if (isset($userIdOfUrl)) {
  10. $userIdOfUrl = $userIdOfUrl->getId();
  11. }
  12.  
  13. return (bool) $userIdOfUrl == $userId;
  14.  
  15. }


$list - nie jest typowana na wejsciu. Co to jest - jakiś obiekt, zmienna skalarna, tablica? Lista czego?
Dalej w tej samej funkcji masz:
  1. $userId = $this->security->getUser();
  2. if (isset($userId)) {
  3. $userId = $userId->getId();
  4. }
  5.  


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:
  1. $userIdOfUrl = $urls->first()->getUserId();

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.
  1. return (bool) $userIdOfUrl == $userId;



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ń.
Go to the top of the page
+Quote Post
Pyton_000
post 24.08.2018, 08:07:15
Post #3





Grupa: Zarejestrowani
Postów: 7 493
Pomógł: 1320
Dołączył: 26.10.2005

Ostrzeżenie: (0%)
-----


Jedna uwaga do Form->isValid. Owszem sprawdza ale rzuca exception. Nawet sama treść wskazuje aby używać wpierw `isSubmitted()`
https://github.com/symfony/form/blob/master/Form.php#L736
Go to the top of the page
+Quote Post
athabus
post 26.08.2018, 17:15:12
Post #4





Grupa: Zarejestrowani
Postów: 874
Pomógł: 44
Dołączył: 2.11.2005
Skąd: Poznań

Ostrzeżenie: (0%)
-----


No proszę, człowiek uczy się całe życie. Zawsze dla czytelności robiłem if (isPost) {} i dlatego ładnie działało samo isValid(), a tutaj faktycznie kolega tego nie robi i u niego wywali wyjątek.
Go to the top of the page
+Quote Post
Pilsener
post 1.09.2018, 22:14:03
Post #5





Grupa: Zarejestrowani
Postów: 1 580
Pomógł: 185
Dołączył: 19.04.2006
Skąd: Gdańsk

Ostrzeżenie: (0%)
-----


Jak powyżej, np. taka metoda:

  1. public function validate($value, Constraint $constraint)
  2. {
  3. if (null === $value || '' === $value) {
  4. return;
  5. }
  6. if (!is_string($value)) {
  7. throw new UnexpectedValueException($value, 'url');
  8. }
  9. $pattern = '/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?/';
  10. if (!preg_match( $pattern, $value, $matches)) {
  11. $this->context->buildViolation($constraint->message)
  12. ->setParameter('{{ url }}', $value)
  13. ->addViolation();
  14. }
  15. }


1. Metoda może rzucić wyjątek, zwrócić null albo być typu void - za dużo
2. Jak już oczekujemy stringa, to czemu nie użyjemy typizowania?
3. Po to jest walidator, żeby nie było wyjątków
4. Zachęcam do używania funkcji "empty" - sprawdza kilka rzeczy za jednym zamachem
5. I nie ma żadnych wbudowanych lub gotowych bibliotek do walidowania URLi?
6. Dobrą praktyką też jest używać return tylko raz i najlepiej na samym końcu metody, poprawia się czytelność i ułatwia debugowanie/rozbudowę, jeśli metoda co chwila nie przerywa swojego biegu w jakiś pozagnieżdżanych ifach

Kolejne rzeczy, które na pierwszy rzut oka mi się nie podobają:
- użycie migracji (to jest dobre gdy projekt jest na produkcji, tutaj nie ma takiej potrzeby, wystarczy wygenerować schemat z encji i użyć fikstur)
- wrzucanie do repo zakomentowanego kodu
- brak instrukcji instalacji apki w readme
- dużo kodu w kontrolerach i używanie ich jak serwisów (np. wystarczy serwis do redirectów zamiast tych dwóch kontrolerów, nie ma też większego sensu tworzyć kontrolera tylko dla jednej metody)
- zbyt opasłe metody, starajmy się zawsze, aby metoda była albo krótka, albo chociaż bardzo wyspecjalizowana


Poza tym całkiem nieźle, widać, że chyba spodobało się Symfony, kto umie taką apkę zrobić nie powinien się chyba za bardzo wstydzić.
Go to the top of the page
+Quote Post
Pyton_000
post 2.09.2018, 09:49:50
Post #6





Grupa: Zarejestrowani
Postów: 7 493
Pomógł: 1320
Dołączył: 26.10.2005

Ostrzeżenie: (0%)
-----


Odniosę się do pkt.6 z którym się nie zgodzę. Nie widzę sensu nie przerywanie programu tak szybko jak to tylko możliwe. Redukuje to ilość ifów które potencjalnie mogą wystąpić. Dużo łatwiej się czyta że np. jak jest teraz zamiast przenosić tego returna na koniec i w całym kodzie szukać gdzie jaki warunek jest spełniony i co to zwraca.
Go to the top of the page
+Quote Post
viking
post 2.09.2018, 09:54:35
Post #7





Grupa: Zarejestrowani
Postów: 4 938
Pomógł: 833
Dołączył: 30.08.2006

Ostrzeżenie: (0%)
-----


Swoją drogą tam wzorzec na url jest niepoprawny. Nie uwzględnia domen np z polskimi znakami ani żadnej współczesnej


--------------------
Go to the top of the page
+Quote Post
athabus
post 2.09.2018, 15:22:32
Post #8





Grupa: Zarejestrowani
Postów: 874
Pomógł: 44
Dołączył: 2.11.2005
Skąd: Poznań

Ostrzeżenie: (0%)
-----


Z if'ami każdy ma swoją teorię. Ja akurat też jestem zwolennikiem przerywania jak najwcześniej. W "Czystym kodzie" był opisywany ten aspekt i tam Marti pisał, że jedno wyjście z funkcji to zaszłość z programowania proceduralnego z długimi i skomplikowanymi funkcjami. Tam faktycznie jeden return miał sens, ale w dobie tanich funkcji, gdzie piszemy (powinniśmy pisać) funkcje, które mają po kilka - kilkanaście liniejek, wg niego kilka wyjść z funkcji poprawia czytelność. Ja osobiście w swoim kodzie staram się jak mogę unikać "else" bo wg mnie psuje czytelność, więc tym bardziej stosuje wczesne returny.

Z drugiej strony rozumiem podejście, że funkcja powinna mieć jedno wyjście - po prostu nie zgadzam się z nim w przypadku krótkich funkcji.

Takie porównanie dla przykładu - funkcja przekształcająca daną wartość na tablicę (tak wiem, że bez sensu)

  1. public function transformToArray($val): array
  2. {
  3. if (is_array($val)) {
  4. return $val;
  5. }
  6.  
  7. if (is_object($var)) {
  8. return $this->transformObjectToArray($val);
  9. }
  10.  
  11. if (is_scalar($var)) {
  12. return $this->transformScalarToArray($val);
  13. }
  14. }
  15.  
  16. public function transformToArray2($val): array
  17. {
  18. $result = null;
  19. if (is_array($val)) {
  20. $result = $val;
  21. } else if (is_object($var)) {
  22. $result = $this->transformObjectToArray($val);
  23. } else {
  24. $result = $this->transformScalarToArray($val);
  25. }
  26.  
  27. return $result
  28. }


Moim zdaniem pierwsza funkcja jest znacznie bardziej czytelna, druga mnie trochę przyprawia o ból głowy. Czytanie pierwszej przerywam w momencie, gdy docieram do interesującego mnie przypadku, a w drugiej muszę scrollować od końca czy jeszcze czegoś nie robię z resultem przed zwróceniem itd. No ale jak pisałem koniec końcu pewnie kwestia preferencji - drugie podejście też ma jakieś poparcie w literaturze.

Go to the top of the page
+Quote Post
Pilsener
post 2.09.2018, 20:07:19
Post #9





Grupa: Zarejestrowani
Postów: 1 580
Pomógł: 185
Dołączył: 19.04.2006
Skąd: Gdańsk

Ostrzeżenie: (0%)
-----


Cytat
druga mnie trochę przyprawia o ból głowy
- bo użyłeś "else if" czego ja np. nie zalecam. Wystarczą te same ify co w funkcji pierwszej, lecz rozbudowane o sprawdzenie czy zmienna "result" jest ustawiona i tyle. Największą dla mnie zaletą tego podejścia jest łatwość debugowania, wystarczy dać:
  1. dump($result);

Identycznie z kodem typu:
  1. return ($a + $b);

Gdzie po return masz blok kodu, niby nic takiego ale jak chcesz prześledzić w debugerze los jakiegoś obiektu to musisz się trochę więcej napocić.

Cytat
Redukuje to ilość ifów które potencjalnie mogą wystąpić
- jest taka zasada w programowaniu "don't sacrifice clarity for brevity", wolę nawet więcej ifów, klamr, brak operatorów trójargumentowych itp. byle kod był jak najłatwiejszy do zrozumienia.
Taki przykład:
  1. public function doSth($x = null){
  2. if($x){
  3. $y = 0;
  4. }else{
  5. $y = $x;
  6. }
  7. }

vs
  1. public function doSth($x = null){
  2. $y = 0;
  3. if($x){
  4. $y = $x;
  5. }
  6. }

W przykładzie pierwszym programista czyta: "blok warunkowy, y jest 0 lub x w zależności od x"
W drugim niby to samo, ale człowiek czyta; "deklarujemy y z domyślną wartością 0, potem nadpisujemy iksem gdy ten nie jest nullem"
Akurat w tych przykładach nie ma to znaczenia, ale za miesiąc te metody mają po 50 linijek i pomimo, że druga metoda była na początku bardziej zwięzła, to nagle okazało się, że y zawsze jest nadpisywany, jest jakiś blok martwego kodu albo odwrotnie, jakiś kod się zawsze wykonuje. Po prostu metoda pierwsza bardziej odwzorowuje jakiś blok decyzyjny opisany w dokumentacji procesu biznesowego, zaś druga choć bardziej zwięzła jest jednocześnie bardziej wrażliwa na zmiany w kodzie (zwłaszcza robione przez wielu programistów).
Typowy tego przykład, to użycie "else if" zamiast zagnieżdżonego ifa - ok, mamy mniej wcięć i linijek, ale czy kod jest dzięki temu bardziej czytelny?
Po prostu uważam, że zwięzłość kodu nigdy nie powinna być ważniejsza niż czytelność i jeśli z jakiś powodów redukcja paru linijek może zaburzyć czytelność to powinniśmy z niej zrezygnować.

No ale to wszystko są szczegóły, które nie mają większego znaczenia jeśli nie tworzy się nie wiadomo jak rozbudowanych metod - niestety w praktyce bywa różnie wink.gif Są o wiele większe problemy w programowaniu
Jakby ktoś chciał zgłębiać ten temat dalej to jedna z ciekawszych debat w sieci o tym:
https://stackoverflow.com/questions/36707/s...eturn-statement
Go to the top of the page
+Quote Post
nospor
post 3.09.2018, 10:53:23
Post #10





Grupa: Moderatorzy
Postów: 34 191
Pomógł: 5664
Dołączył: 27.12.2004




Cytat
- jest taka zasada w programowaniu "don't sacrifice clarity for brevity", wolę nawet więcej ifów, klamr, brak operatorów trójargumentowych itp. byle kod był jak najłatwiejszy do zrozumienia.
Taki przykład:
Pilsener odnosisz sie do wypowiedzi Pytona wyrwanej z kontekstu i jako kontrargument podajesz przyklad totalnie z 4 liter. No tak troche nieladnie. Pyton mowil o redukcji IFow spowodowanej szybszym wyjsciem z funkcji. Ty zas kontrargumentujesz podajac zupelnie inny przyklad majacy sie nijak do tego co mowil Pyton

Ja tez wychodze z funkcji jesli to mozliwe bo redukuje to zbedne zagniezdzenia pozniej. To wlasnie sprawia kod czytelniejszym o co i zdaje sie tobie rowniez zalezy.

Cytat
- bo użyłeś "else if" czego ja np. nie zalecam. Wystarczą te same ify co w funkcji pierwszej, lecz rozbudowane o sprawdzenie czy zmienna "result" jest ustawiona i tyle.
Czyli wg twoich wskazowek kod bedzie wygladal tak

  1. public function transformToArray2($val): array
  2. {
  3. $result = null;
  4. if (is_array($val)) {
  5. $result = $val;
  6. }
  7. if ($result === null && is_object($var)) {
  8. $result = $this->transformObjectToArray($val);
  9. }
  10. if ($result === null) {
  11. $result = $this->transformScalarToArray($val);
  12. }
  13.  
  14. return $result
  15. }
  16.  

Czy to jest bardziej czytelne od kodu z kilkoma return? No ja tez uwazam ze nie. Juz nie wspominajac o tym ze kod musi wykonac pare dodatkowych sprawdzan za kazdym razem. Krotko rzecz mowiac same minusy wink.gif Argument za debugowaniem w tym momencie uwazam za nietrafiony. Kodu nie powinno sie pisac by moc latwo dac dumpa tylko by byl czytelny i efektywny.

Cytat
Gdzie po return masz blok kodu, niby nic takiego ale jak chcesz prześledzić w debugerze los jakiegoś obiektu to musisz się trochę więcej napocić.

Czyli zamiast prostego:

  1. return ($a+$b);


mamy tworzyc dodatkowa zmienna
  1. $c = $a+$b;
  2. return $c;

tylko dlatego ze moze kiedys ktos tam bedzie chcial zrobic dumpa z dzialania $a+$b. No prosze cie. wink.gif


--------------------

Najlepsze kawałki programistyczne || Dowcipy o informatykach || Forum PHP dla opornych
Klasy: Pager (stronicowanie) | Cache | ShoutBox (Chat) | Widok | Ładne url'e

"Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista
"Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer

Go to the top of the page
+Quote Post
athabus
post 3.09.2018, 15:07:55
Post #11





Grupa: Zarejestrowani
Postów: 874
Pomógł: 44
Dołączył: 2.11.2005
Skąd: Poznań

Ostrzeżenie: (0%)
-----


Kurcze Pilsener dla mnie 2 wersja jest bardziej czytelna. Może dlatego, że w pierwszej zgubiłeś ! w pierwszym warunku i się zastanawiałem co ta funkcja robi. Ale nawet bez tego jakoś przyzwyczaiłem się do drugiego zapisu i tak bym to zrobił... Widać wszystko kwestią gustu i coś dla jednych jest bardziej czytelne niż dla innych.

BTW poruszyłeś ciekawy temat, który uciekł w toku dyskusji, czyli CQS - command-guery separation. Ostatnio staram się stosować (ostatnio w sumie całe swoje kodowe nawyki zmieniam) i zauważyłem, że czasami powoduje puchnięcie kodu (zwłaszcza jak funkcja na wyższym poziomie grupuje działania wykonywane przez funkcje na niższym poziomie, gdzie część jest typu query, a inne typu command). Stosujecie tą regułę bez odstępstw, czy Wam się jednak zdarza. Ja staram się bez odstępstw, bo uznałem, że czytelny kod czasami może być dłuższy niż ten nieczytelny.
Go to the top of the page
+Quote Post
Pilsener
post 4.09.2018, 09:45:20
Post #12





Grupa: Zarejestrowani
Postów: 1 580
Pomógł: 185
Dołączył: 19.04.2006
Skąd: Gdańsk

Ostrzeżenie: (0%)
-----


Cytat
odnosisz sie do wypowiedzi Pytona wyrwanej z kontekstu i jako kontrargument podajesz przyklad totalnie z 4 liter. No tak troche nieladnie.
- hehe, chciałem tylko rozwinąć temat i skopiowałem potrzebne zdanie, potem pomyślałem, że nieładnie tak kopiować czyjąś pracę, więc dałem w cytat i tak jakoś wyszło nerdsmiley.png

I oczywiście masz rację Nospor z tymi przykładami, ja jestem przeciwnikiem jakiś sztywnych reguł typu "zawsze jeden return".
A z tym kodem po return to tak tylko na początku zazwyczaj wygląda, za jakiś czas masz już cały blok kodu tam a to już wkurza. Jak masz tego typu "szkielet" to potem nikomu się nie będzie chciało tego refaktorować, programista to zwierzę dość leniwe, zazwyczaj zmienia kod po najmniejszej linii oporu i w taki sposób można natworzyć miejsc, gdzie kod się będzie rozwijał w złą stronę i w sposób niezbyt kontrolowany.

Poza tym pewne rzeczy robi się niejako automatycznie gdy piszesz kod jak Boziu kazała, czyli zaczynasz od testów. Robisz test, potem metodę i od razu deklarujesz zmienną, zwracasz ją i zapuszczasz test - potem rozbudowujesz tak powstały szkielet metody co wymusza niejako nieco inne podejście. Tylko znów ilu programistów tak robi? W dobie fw i bibliotek testowanie jednostkowe co chwila się zmieniającej logiki biznesowej nie ma większego sensu.

Cytat
Może dlatego, że w pierwszej zgubiłeś ! w pierwszym warunku


Bo tu masz kolejny przykład:
  1. if(!$sth)

vs
  1. if($sth == false)


Te wykrzykniki tak na pierwszy rzut oka zbyt widoczne nie są smile.gif Zwłaszcza jak stosujesz niezależnie tak zwane "rzutowanie na booleana" (dużo osób ma ten nawyk)
  1. if(!!$sth)


Cytat
CQS - command-guery separation
- tak, to też jest dobre, wpisuje się w generalną zasadę by metody były jak najbardziej wyspecjalizowane. Tylko jakbyś zebrał do kupy te wszystkie pryncypia, to nie byłoby w głowie miejsca na programowanie smile.gif

Generalnie warto poczytać o jakiś zasadach "czystego kodu" i starać się ich trzymać, nie dać się zwariować smile.gif

Ten post edytował Pilsener 4.09.2018, 09:47:42
Go to the top of the page
+Quote Post
athabus
post 4.09.2018, 19:29:26
Post #13





Grupa: Zarejestrowani
Postów: 874
Pomógł: 44
Dołączył: 2.11.2005
Skąd: Poznań

Ostrzeżenie: (0%)
-----


Zgadzam się, że nie można na zasady patrzeć ślepo (zwłaszcza, że w praktyce często zasady okazują się wewnętrznie sprzeczne), ale akurat CQS jest taką zasadą, że albo się ją stosuje albo nie - tak mi się przynajmniej wydaje. Jeśli stosuje się ją tam, gdzie wygodnie to traci sens bo wracamy do punktu wyjścia i nadal nie wiemy czy metoda jest zapytaniem czy komendą bez zajrzenia w jej kod. Chyba nawet stosowanie jej w 95% jest gorsze niż nie stosowanie, bo wtedy ma się wrażenie że coś jest np. komendą, a nagle okazuje się mutować obiekt. Gdy jej nie stosujemy to przynajmniej wiemy, że w tym zakresie mamy w kodzie burdel i wszystko trzeba dokładnie sprawdzić... Takie jest moje zdanie, choć praktyki z CQS mam mało - nowy nabytek w arsenale ;-).
Go to the top of the page
+Quote Post

Reply to this topicStart new topic
1 Użytkowników czyta ten temat (1 Gości i 0 Anonimowych użytkowników)
0 Zarejestrowanych:

 



RSS Wersja Lo-Fi Aktualny czas: 19.09.2018 - 14:35