commentbox - ocena paczki, co poprawić, laravelowa paczka Opcje

[starf]

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 (IMG:style_emoticons/default/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.

[nospor]

Bbyloby naprawde milo, jakbys jeszcze napisal co ta paczka robi (IMG:style_emoticons/default/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 (IMG:style_emoticons/default/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:

[PHP] pobierz, plaintext 
try {
            $articles = Article::all();
        } catch (ModelNotFoundException $e) {
            $restExceptionHandeler = new RestExceptionHandler();
            return $restExceptionHandeler->render($e);
        }
[PHP] pobierz, plaintext 

Wygladaja poprostu tak:

[PHP] pobierz, plaintext 
            $articles = Article::all();
[PHP] pobierz, plaintext 

Prawda ze ladniej i czytylniej ? (IMG:style_emoticons/default/wink.gif)

[starf]

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 (IMG:style_emoticons/default/smile.gif)

[nospor]

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

[starf]

to żebym dobrze zrozumiał, jeśli zrobimy tak jak pisałeś po prostu np tylko

[PHP] pobierz, plaintext 
$article = Article::findOrFail($id);
[PHP] pobierz, plaintext 

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'

[nospor]

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

[starf]

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 ?

[nospor]

Wybacz, wlasnie wrocilem z tzw "welcome drinks" i moge troche niekontakowac (IMG:style_emoticons/default/wink.gif)

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 (IMG:style_emoticons/default/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 (IMG:style_emoticons/default/wink.gif) Za godzine bede ok (IMG:style_emoticons/default/wink.gif)

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 (IMG:style_emoticons/default/smile.gif)

[starf]

A i widzisz i teraz rozumiem o co chodzi (IMG:style_emoticons/default/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 ?

[nospor]

"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 (IMG:style_emoticons/default/wink.gif)

[starf]

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 (IMG:style_emoticons/default/smile.gif) akurat tu mi zależy żeby było spoko bo to tak hobbysticznie, w pracy to wiadomo jak to wygląda.

[r4xz]

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) (IMG:style_emoticons/default/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 (IMG:style_emoticons/default/smile.gif)

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

[starf]

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

[viking]

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?

[nospor]

@viking jak cos bedzie nie tak to poleci wyjatek a nie result 200

[viking]

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

[nospor]

Nie wiem co zroci laravel. Na pewno nie zwroci 200 jak pisales (IMG:style_emoticons/default/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 (IMG:style_emoticons/default/smile.gif)