![]() |
![]() ![]() |
![]() |
![]()
Post
#1
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
Hej,
Ogólnie od jakiegoś czasu pracuję jako programista, ale jako że nie mam nikogo kto by mi zrobił jakikolwiek codereview albo pomógł powiedział co poprawić to stwierdziłem że zapytam tutaj ![]() Jak każdy szanujący się programista staram się rozwijać samodzielnie itp. Postanowiłem napisać własną paczkę do Laravela, i bardzo bym prosił nie tyle o ocene bo mam świadomość tego że nie koniecznie pięknie jest napisane, samemu mi się nie podoba to raczej wiadomości co mógłbym zmienić poprawić no i oczywiście jak. Podsyłam link https://github.com/19lyczkul91/CommentBox ponadto zależy mi też żeby ktoś mi doradził (tutorial lub po prostu napisał) jak pisać testy jednostkowe. Pod jak nie mam na myśli coś w stylu "O Panie tu musisz wywołać tą metodę i zawsze ma Ci zwracać to i to" tylko raczej coś w stylu tu powinieneś napisać testy, tu nie. Konkretne moje pytanie czy to metod store/update itp powinno się robić testy ? I jeśli tak to jak zrobić np do metody store test żeby nie utworzyło nowego elementu ? nie wiem co jeszcze jak wymyślę to dodam. |
|
|
![]()
Post
#2
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Bbyloby naprawde milo, jakbys jeszcze napisal co ta paczka robi
![]() W kontrolerze stworzyles metode renderError(), ktora raz uzywasz a raz nie. Widac, ze stworzyles ja w polowie kodu a potem zapomniales uzyc wszedzie ![]() Generalnie nie lubie, lapac wyjatkow jak ty to robisz i za kazdym razem zwracac to samo. Zbedna duplikacja kodu. W laravel na pewno jest jakis ExceptionHandler, w ktorym to jedbym mozesz to wszystko obsluzyc. Wowczas kody jak ten:
Wygladaja poprostu tak:
Prawda ze ladniej i czytylniej ? ![]() -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#3
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
No tak, paczka ma za zadanie dać możliwość dodawania artykułów i do nich komentarzy ew. komentarzy do komentarzy itp itd, apropo ExceptionHandler, to ma być RestApi więc chyba ExceptionHandler nam odpada ? dodałem już to render error wszędzie
![]() |
|
|
![]()
Post
#4
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Cytat być RestApi więc chyba ExceptionHandler nam odpada I czemy RestApi nie moze miec ExceptionHandlera? Taka sama aplikacja jak kazda inna, ino ze zamiast html zwraca JSON
-------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#5
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
to żebym dobrze zrozumiał, jeśli zrobimy tak jak pisałeś po prostu np tylko
wyrzuci nam tutaj całego htmla z jakimś tam errorem to jak obsłużyć na froncie errory ? Myślałem że to trzeba zwrócić coś w stylu error => 'nie działa' code => '500' |
|
|
![]()
Post
#6
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Tak, ale ten wyjatek moze byc wylapany w ExceptionHandler i on moze zwrocic to co trzeba. Nie widze powodu by za kazdym razem robic to w akcji controllera. Tylko zbedny kod
-------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#7
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
Czyli żebym dobrze zrozumiał, nie przejmuję się jakimś wyrzucaniem kodu 404 i message nie ma takiego czegoś tam tylko po prostu jak coś to wywali nam całego html-a a front niech sobie radzi jak chce tak ?
Jak będziesz mieć chwilę to zerknij teraz na kod. trochę pozmieniałem powywalałem co niepotrzebne itp. I teraz jeszcze pytanie o testy . Tak jak w pierwszym poście jak zrobić testy do store tak żeby nie utworzyć nowego rekordu ? |
|
|
![]()
Post
#8
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Wybacz, wlasnie wrocilem z tzw "welcome drinks" i moge troche niekontakowac
![]() Cytat Czyli żebym dobrze zrozumiał, nie przejmuję się jakimś wyrzucaniem kodu 404 i message nie ma takiego czegoś tam tylko po prostu jak coś to wywali nam całego html-a a front niech sobie radzi jak chce tak ? Ale czemu html? Lapales wyjatek po czym zwracales odpowiedni response. Teraz masz robic dokladnie to samo ale w ExceptionListener. Patrzac na twoj kod teraz, to widze ze wywaliles te try catch, ale nie dodales ExceptionListener wiec teraz jest jeszcze gorzej ![]() Musisz miec globalby ExceptionListener, ktory bedzie lapal te wyjatki i zwracal to co trzeba. Chyba, ze zaczynam cos przegapiac bo tak jak mowilem troche juz nie kontaktuje ![]() ![]() Cytat Tak jak w pierwszym poście jak zrobić testy do store tak żeby nie utworzyć nowego rekordu ? A to juz musza sie wypowiedziec madrzejsi ode mnie. Sam z checia poslucham ![]() -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#9
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
A i widzisz i teraz rozumiem o co chodzi
![]() |
|
|
![]()
Post
#10
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Cytat "do standardowych metod w kontrolerach nie robi się testów" prawda fałsz ? Nie wiem jak w Laravel ale w Symfony masz standardowy testy na akcje, ktore sprawdzaja czy jest dany tekst w response. Czy to potrzebne? Czasami moze sie przydac, ale z braku czasu... wiesz ![]() -------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#11
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
w laravelu też mam testy na akcje ale... za ostatnim razem mi wywoływały akcje i dodawał mi się nowy rekord do bazy... a to było co najmniej głupie, może ja wtedy nie umiałem
![]() |
|
|
![]()
Post
#12
|
|
![]() Grupa: Zarejestrowani Postów: 673 Pomógł: 106 Dołączył: 31.12.2008 Ostrzeżenie: (0%) ![]() ![]() |
To ja dorzucę jeszcze swoje rady/uwagi/spostrzeżenia:
1. Zdecydowanie bardziej lubię zapis return $this->hasMany(Comment::class); - niby niewiele, a znacznie upraszcza refactoring. 2. Te app->make tutaj całkowicie zbędne, zgodzisz się? Do tego include powyżej też mam mieszane uczucia skoro w boot je także załączasz (już poprawnie) ![]() 3. Tutaj informacja odnośnie testowania i zmiany stanu bazy po wykonaniu testów (strategia z migracjami i transakcjami). Niestety sprawa się komplikuje w przypadku bibliotek. 4. W composer nie określaj wersje, wrzucisz na packagist to pójdzie po tagach. 5. Próbowałeś to załączyć? Ustaw w composer autoload psr-4 6. Readme, readme, readme! Co do testów to hmm... trochę mnie zaskoczyliście z tym testowaniem metod kontrolerów, zawsze mi się wydawało że jak się nie ma zbytnio czasu to robi się właśnie takie podstawowe testy - czy po wejściu na daną stronę widzimy tekst taki i taki, a po wypełnieniu formularza przekieruje nas na określony adres i utworzy wpis w bazie itd. (czyli raczej odpuszczamy jednostkowe i bardziej wchodzimy w integracyjne nastawione na ogólne testowanie systemu). Jako ciekawostka rozszerzenie do Chrome. Odnośnie dyskusji z ExceptionHandlerem - zerknij w szczególności na metodę render. PS Jak dla mnie wygląda to całkiem dobrze ![]() Ten post edytował r4xz 7.04.2017, 21:09:34 -------------------- |
|
|
![]()
Post
#13
|
|
Grupa: Zarejestrowani Postów: 7 Pomógł: 0 Dołączył: 5.04.2017 Ostrzeżenie: (0%) ![]() ![]() |
poprawiłem trochę z tego co pisałeś, nie rozumiem kompletnie idei listenerów w Laravelu, jakoś z Symfony pyknąłem od kopa listenery a tutaj czarna magia
|
|
|
![]()
Post
#14
|
|
Grupa: Zarejestrowani Postów: 6 365 Pomógł: 1114 Dołączył: 30.08.2006 Ostrzeżenie: (0%) ![]() ![]() |
Nie znam co prawda LV ale co się stanie jak https://github.com/lyczkul/commentbox/blob/...troller.php#L46 sypnie błędem? Zwrócisz OK chociaż nie jest OK. Analogicznie w kilku miejscach. Nie powinieneś dodać PSR-4 i ścieżki w composer.json?
-------------------- |
|
|
![]()
Post
#15
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
@viking jak cos bedzie nie tak to poleci wyjatek a nie result 200
-------------------- "Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista "Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer |
|
|
![]()
Post
#16
|
|
Grupa: Zarejestrowani Postów: 6 365 Pomógł: 1114 Dołączył: 30.08.2006 Ostrzeżenie: (0%) ![]() ![]() |
A co wróci wtedy do klienta? Na ok jest json z danymi. A na fail?
-------------------- |
|
|
![]()
Post
#17
|
|
![]() Grupa: Moderatorzy Postów: 36 455 Pomógł: 6292 Dołączył: 27.12.2004 ![]() |
Nie wiem co zroci laravel. Na pewno nie zwroci 200 jak pisales
![]() Dlatego mowilem od samego poczatku, ze ma byc jeszcze globalny exception listener, ktory bedzie lapal te wyjatki i zwracal wlasciwy JSON. W piewszej wersji skryptu autor sam za kazdym razem lapal wyjatek w kazdej akcji kontrolera i zwracal wlasciwy response. Ale moim zdaniem to jest zbedna duplikacja kodu. po to wymyslono exception listener by nie tworzyc takich potworkow ![]() -------------------- "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: 26.04.2024 - 10:03 |