URL shortener - Symfony 4. |
URL shortener - Symfony 4. |
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 Czym więcej feedbacku, tym lepiej! https://github.com/selfmadeking/URL-shortener-Symfony-4 |
|
|
23.08.2018, 21:25:44
Post
#2
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 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.
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.
$list - nie jest typowana na wejsciu. Co to jest - jakiś obiekt, zmienna skalarna, tablica? Lista czego? Dalej w tej samej funkcji masz:
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:
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.
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ń. |
|
|
24.08.2018, 08:07:15
Post
#3
|
|
Grupa: Zarejestrowani Postów: 8 068 Pomógł: 1414 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 |
|
|
26.08.2018, 17:15:12
Post
#4
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 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.
|
|
|
1.09.2018, 22:14:03
Post
#5
|
|
Grupa: Zarejestrowani Postów: 1 590 Pomógł: 185 Dołączył: 19.04.2006 Skąd: Gdańsk Ostrzeżenie: (0%) |
Jak powyżej, np. taka metoda:
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ć. |
|
|
2.09.2018, 09:49:50
Post
#6
|
|
Grupa: Zarejestrowani Postów: 8 068 Pomógł: 1414 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.
|
|
|
2.09.2018, 09:54:35
Post
#7
|
|
Grupa: Zarejestrowani Postów: 6 375 Pomógł: 1116 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
-------------------- |
|
|
2.09.2018, 15:22:32
Post
#8
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 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)
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. |
|
|
2.09.2018, 20:07:19
Post
#9
|
|
Grupa: Zarejestrowani Postów: 1 590 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ć:
Identycznie z kodem typu:
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:
vs
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 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 |
|
|
3.09.2018, 10:53:23
Post
#10
|
|
Grupa: Moderatorzy Postów: 36 523 Pomógł: 6309 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. 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 PytonTaki przykład: 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
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 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:
mamy tworzyc dodatkowa zmienna
tylko dlatego ze moze kiedys ktos tam bedzie chcial zrobic dumpa z dzialania $a+$b. No prosze cie. -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
3.09.2018, 15:07:55
Post
#11
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 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. |
|
|
4.09.2018, 09:45:20
Post
#12
|
|
Grupa: Zarejestrowani Postów: 1 590 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 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:
vs
Te wykrzykniki tak na pierwszy rzut oka zbyt widoczne nie są Zwłaszcza jak stosujesz niezależnie tak zwane "rzutowanie na booleana" (dużo osób ma ten nawyk)
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 Generalnie warto poczytać o jakiś zasadach "czystego kodu" i starać się ich trzymać, nie dać się zwariować Ten post edytował Pilsener 4.09.2018, 09:47:42 |
|
|
4.09.2018, 19:29:26
Post
#13
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 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 ;-).
|
|
|
23.09.2018, 09:30:48
Post
#14
|
|
Grupa: Zarejestrowani Postów: 148 Pomógł: 14 Dołączył: 23.02.2013 Ostrzeżenie: (0%) |
Po pierwsze bardzo nie podoba mi się brak typowania zmiennych i pól w klasie.
Dlaczego pola klasy nie mają docka z typami? Przecież korzystasz z phpstorma, a on potrafi wygenerować - stajesz na parametrach konstruktora i klikasz alt+enter.... PHPStorm poradzi sobie z wykryciem typów pól określonych w konstruktorze. Dodawanie docblocków w tym przypadku jest nadmiarowe. Przy okazji, wrzucanie do repo informacji o ide tylko je zaśmieca. -------------------- |
|
|
24.09.2018, 14:45:59
Post
#15
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 Dołączył: 2.11.2005 Skąd: Poznań Ostrzeżenie: (0%) |
Ciekawe, że każdy ma swoje standardy w kwestii kodu.
Ja bym osobiście jednak komentarzy przy polach klasy nie uznał za nadmiarowe, bo to że ide rozpozna to jedna sprawa, a czytelność kodu to druga. Wchodząc do takiej klasy po miesiącu skąd mam wiedzieć jaki jest typ pola - musiałbym sprawdzić w konstruktorze / w innym miejscu, gdzie jest ustawiana wartość pola. Tak więc moim zdaniem jednak warto docka dać. Nadmiarowy to wg mnie taki, który powiela informacje z sygnatury funkcji bezpośrednio nad nią np: Kod /** * @param string $name **/ function setName(string $name){} Inna sprawa, że idąc tym tokiem rozumowania w zasadzie można by nie definiować w ogóle pól w klasie bo ide i bez tego sobie poradzi, a w samych PHP nie ma wymogu aby pola były zdefiniowane - można je tworzyć dynamicznie. Co do wrzucania do repo informacji o ide to nie rozumiem o co chodzi - chyba że to nie było do mnie. |
|
|
24.09.2018, 14:49:29
Post
#16
|
|
Grupa: Moderatorzy Postów: 36 523 Pomógł: 6309 Dołączył: 27.12.2004 |
Na szczescie w nowym wydaniu php bedzie mozna typowac rowniez i wlasciwosci i nie bedzie juz takich rozterek
-------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
24.09.2018, 14:54:18
Post
#17
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 Dołączył: 2.11.2005 Skąd: Poznań Ostrzeżenie: (0%) |
Chyba czytasz w moich myślach, bo właśnie miałem napisać "ciekawe czy kiedyś wprowadzą opcjonalne typowanie pól" ;-)
No to jeszcze poproszę return type mixed przy funkcji i będzie super. |
|
|
24.09.2018, 15:03:38
Post
#18
|
|
Grupa: Moderatorzy Postów: 36 523 Pomógł: 6309 Dołączył: 27.12.2004 |
Cytat No to jeszcze poproszę return type mixed przy funkcji i będzie super. Oczadzial? Nie po to wprowadzono typowanie by teraz wszystko do kosza wyrzucic. -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
24.09.2018, 16:29:57
Post
#19
|
|
Grupa: Zarejestrowani Postów: 898 Pomógł: 48 Dołączył: 2.11.2005 Skąd: Poznań Ostrzeżenie: (0%) |
Czasami sa sytuacje, ze nie da sie w php przewidziec zwracanego typu. Np. dzisiaj budowalem konfiguracje do kontenera DI i odczyt wartosci parametru mogl zwracac wszystko od inta po inny obiekt. Nie da sie takiego czegos opisac bez doca, a brak zwracanego typu moze oznaczac, ze albo ktos nie oznaczyl typu, albo funkcja nic nie zwraca. Ja w takich sutuacjach zawsze daje doca return mixed aby uniknac dwuznacznosci i wolalbym to jawnie zdeklarowac dokladnie tak samo jak :void, gdy nic nie zwracamy.
Idealnie by bylo gdyby doc byl uzywany tylko, gdy trzeba dac cos wiecej niz to co mozna zawrzec w sygnaturze. |
|
|
24.09.2018, 18:41:44
Post
#20
|
|
Grupa: Moderatorzy Postów: 36 523 Pomógł: 6309 Dołączył: 27.12.2004 |
Jesli nie wiesz, co bedzie zwracac twoja funkcja, znaczy ze masz zle zaprojektowany system
W nowym php tez maja wprowadzic typ OBJECT ktory bedzie ogolnie obiektem nie wazne jakim byle obiektem. Moze to ci pomoze troche ogarnac to co tam masz u siebie -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
Wersja Lo-Fi | Aktualny czas: 1.11.2024 - 00:32 |