Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> commentbox - ocena paczki, co poprawić, laravelowa paczka
starf
post 6.04.2017, 09:14:39
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 smile.gif
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.
Go to the top of the page
+Quote Post
nospor
post 6.04.2017, 09:44:08
Post #2





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




Bbyloby naprawde milo, jakbys jeszcze napisal co ta paczka robi smile.gif

W kontrolerze stworzyles metode renderError(), ktora raz uzywasz a raz nie. Widac, ze stworzyles ja w polowie kodu a potem zapomniales uzyc wszedzie wink.gif

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:

  1. try {
  2. $articles = Article::all();
  3. } catch (ModelNotFoundException $e) {
  4. $restExceptionHandeler = new RestExceptionHandler();
  5. return $restExceptionHandeler->render($e);
  6. }


Wygladaja poprostu tak:
  1. $articles = Article::all();

Prawda ze ladniej i czytylniej ? wink.gif


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

"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
starf
post 6.04.2017, 11:45:57
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 smile.gif
Go to the top of the page
+Quote Post
nospor
post 6.04.2017, 12:14:32
Post #4





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
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

Go to the top of the page
+Quote Post
starf
post 6.04.2017, 13:35:27
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

  1. $article = Article::findOrFail($id);


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'
Go to the top of the page
+Quote Post
nospor
post 6.04.2017, 14:21:40
Post #6





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
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

Go to the top of the page
+Quote Post
starf
post 6.04.2017, 14:26:07
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 ?
Go to the top of the page
+Quote Post
nospor
post 6.04.2017, 14:33:34
Post #8





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




Wybacz, wlasnie wrocilem z tzw "welcome drinks" i moge troche niekontakowac wink.gif

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 wink.gif
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 wink.gif Za godzine bede ok wink.gif

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 smile.gif


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

"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
starf
post 6.04.2017, 14:39:16
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 biggrin.gif pyknę może jeszcze dzisiaj to się bd odzywać. W ogóle raz się spotkałem z twierdzeniem "do standardowych metod w kontrolerach nie robi się testów" prawda fałsz ?
Go to the top of the page
+Quote Post
nospor
post 6.04.2017, 14:46:23
Post #10





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
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 wink.gif


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

"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
starf
post 6.04.2017, 16:00:37
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 smile.gif akurat tu mi zależy żeby było spoko bo to tak hobbysticznie, w pracy to wiadomo jak to wygląda.
Go to the top of the page
+Quote Post
r4xz
post 7.04.2017, 21:05:54
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) smile.gif
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 smile.gif

Ten post edytował r4xz 7.04.2017, 21:09:34


--------------------
Go to the top of the page
+Quote Post
starf
post 8.04.2017, 13:35:07
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
Go to the top of the page
+Quote Post
viking
post 8.04.2017, 13:40:53
Post #14





Grupa: Zarejestrowani
Postów: 6 365
Pomógł: 1113
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?


--------------------
Go to the top of the page
+Quote Post
nospor
post 8.04.2017, 19:43:11
Post #15





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
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

Go to the top of the page
+Quote Post
viking
post 8.04.2017, 20:12:19
Post #16





Grupa: Zarejestrowani
Postów: 6 365
Pomógł: 1113
Dołączył: 30.08.2006

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


A co wróci wtedy do klienta? Na ok jest json z danymi. A na fail?


--------------------
Go to the top of the page
+Quote Post
nospor
post 8.04.2017, 20:15:12
Post #17





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




Nie wiem co zroci laravel. Na pewno nie zwroci 200 jak pisales wink.gif
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 smile.gif


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

"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

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.03.2024 - 05:56