Code review niekompletnego projektu w Springu MVC

0

Cześć.
Jestem samoukiem, od dobrych 3 miesięcy uczę się programowania, a teraz przyszło mi napisać pierwszy w życiu projektu w Spring MVC. Stoję teraz w miejscu, bo do głowy przychodzą coraz to nowe pytania i wątpliwości.
Tutaj link do GitHub żeby było wiadomo o czym piszę:
https://github.com/sylwekp/TodoApp
Na razie dodałem tylko rejestrację, logowanie, przypominanie hasła na e-mail, a także wyświetlanie listy zadań z paginacją.
Do zrobienia mam: sortowanie wyników przy pomocy przycisku DropDown, dodawanie, edytowanie, usuwanie zadań, oznaczanie ich jako wykonane, a także jakiś mały panel admina.

Pytania w związku z powyższym:

  1. Czy ten kod jakoś wygląda, tzn. ma ręce i nogi? Nazwy metod, zmiennych, podział na pakiety?
  2. DTO przy takiej "aplikacji" jest konieczne? Encje nie mają dużo pól, przy rejestracji użytkownika w takie pola jak, np. enabled, role wpisywałbym wartości po prostu w serwisie, żeby wszystko było pełne.
  3. Czy takie coś: https://github.com/sylwekp/TodoApp/blob/master/src/main/java/pl/sylwekp/todolist/controller/UserController.java#L48 jest w porządku? Jeśli nie to jak inaczej do tego podejść, jakieś wskazówki?
  4. Usuwanie ToDo: nie wiem jak się do tego zabrać. Jeśli zrobiłbym controller z metodą która mapuje na .../users/todos/todo/{id}/delete i zawiera w sobie ...todoService.delete(id) to przecież inny zalogowany użytkownik mógłby wpisać sobie URL z dowolnym ID i mógłby usunąć zadanie innego użytkownika. Musiałbym dodać metodę zwracającą true/false czy to zadanie aktualnie zalogowanego użytkownika. Gdzie to więc sprawdzić? W kontrolerze czy metoda w serwisie:
    todoService.delete(principal.getName(), id) -> tam pobranie zadania na podstawie id i przyrównanie nicku rzeczywistego właściciela z tym który przesłałem z controllera.
    Jeśli tak to wyrzucić wyjątek podobnie jak w pytaniu 3 i go obsłużyć?
  5. Jeśli miałbym już zostać przy DTO to rozmyślam też nad edycją zadania. Dodawanie wiadomo - przez DTO podobnie jak rejestracja użytkownika, ale jak podejść do edytowania?
    Z poziomu kontrolera todoService.edit(id) -> w serwisie w tej metodzie pobieranie z repozytorium zadania, mapowanie go na DTO i przesyłanie go z powrotem do kontrolera, następnie dodawanie do modelu i zwracanie widoku, coś takiego?
    A, musiałbym też sprawdzić podobnie jak w pytaniu 4, czy jest to zadanie tego konkretnego użytkownika i też: wywalić wyjątek?
  6. Sprawa commitów. Nazewnictwo w miarę? Jeden commit = 1 funkcjonalność, poprawka? Jeśli dodaję mały opis to zaczynać od 3 linijki, a drugą pozostawić pustą, tak?

Na razie tyle. Śmiało krytykować i pisać jakie babole narobiłem, wtedy wezmę się za poprawki. Projekt ten docelowo ma mi posłużyć przy aplikacji do pierwszej pracy, bądź stażu/praktyki, stąd te wszystkie pytania i rozkminy. Za każdą odpowiedź z góry dzięki :)

0

Podbijam.
Nikt, nic? :o
Pytanie 4 i 5 w sumie odpada, wymyśliłem rozwiązanie które dopiero będę implementować.

1

Cześć,
generalnie przejrzałem kod tylko na szybko, bez dokładnego przyglądaniu się każdej klasie. Moich kilka rad/uwag:

  1. Jak na pierwszy projekt to wygląda dość czytelnie i trzyma się całości.W niektórych miejscach są dziwne nazwy metod. Jeżeli chodzi o pakiety to w małym projekcie można powiedzieć, że taki podział jest ok. Jeżeli robisz większy projekt to podział ten raczej się nie sprawdza. Wyobraź sobie pakiet controller i np. 30 klas w środku. Ciężko się na to patrzy i czegoś szuka. Stosuje się raczej pierwszy podział per feature itp.
  2. W skrócie to - tak. Staraj się odseparować warstwę encji modelowych od takich obiektów, które przesyłasz dalej. Często encje mają więcej pól niepotrzebnych itp. Przepychanie encji przez wszystkie warstwy raczej nie jest dobrym rozwiązaniem i doprowadzasz do architektury "Encje na twarz i pchasz" - Paweł Szulc ;)
  3. Nie jest w porządku. Nigdy nie myśl o tym aby łapać run-time'owe wyjątki. Zły nawyk, który później może doprowadzić ,że zaczniesz łapać NullPointerException i będziesz coś próbował z tym zrobić itp. Druga sprawa nie rób tak:
} catch (Exception e) {

Prowadzi to do nieprzewidzianych efektów i jest bardzo zła praktyką. Jezeli już to zdefiniuj swój wyjątek i próbuj go obsłużyć w tym miejscu albo w jakimś ExceptionHandlerze.

Inne uwagi:

  • Widze ,że robisz interfejs i później jedną implementację do tego. Mam nadzieje, że wiesz po co to robisz i nie podążasz bezmyślnie za tutorialami. Poczytaj o tym ;)
  • Zainteresuj się biblioteką Lombok (odpada Ci pisanie setterów, getterów, generowanie loggerów, konstruktorów itp)
  • Nie pisz metod w stylu byteToHex. Użyj DataTypeConverter lub innej dostępnej.
  • Jeżeli robisz projekt aby móc to komuś pokazać to może zainteresuj się rozwiązaniem typu restowe api (json). JSP jest bardzo przestarzałą technologią i ogranicza Twój backend w pewien sposób. Dobre restowe api daje Ci swobodę jeżeli będziesz chciał zrobić do tego apkę mobilną czy całkowicie zmienić widok na js.
  • Ostatni, ale bardzo ważny punkt jeżeli będziesz chciał to komuś pokazać to zacznij pisać testy. Będzie to o Tobie dobrze świadczyć, dodatkowo jeżeli trafisz do normalnej firmy to w pracy Twój kod bez testów nie zostanie dopuszczony.

Tyle na szybko.
Pozdrawiam

0

Nocny Marek: dzięki za feedback! :)

  1. Nad nazwami metod posiedzę potem. Spróbuję spojrzeć na ten kod jakby był pisany przez inną osobę. Z kontrolerami zapamiętam, chociaż takie rzeczy załapię pewnie na pierwszym stanowisku pracy.
  2. Okej. Znalazłem też jego (Pawła Szulca) prezentacje na YouTube na ten temat - obejrzę.
  3. To utworzę swoje wyjątki, ale obsłużyć i tak chcę w tym miejscu żeby z powrotem zwrócić widok rejestracji z błędem że np. taki login jest już zajęty.

Uwagi:

  • Z grubsza wiem o co chodzi, ale jeszcze doczytam :)
  • Poczytam o Lombok.
  • Właśnie próbowałem znaleźć gotowej implementacji kodowania do SHA1 (żeby wygenerować kod do resetowania hasła), ale za cholerę nie znalazłem... Musiałbym dorzucać nowe biblioteki, a nie chciałem dla tej jednej metody. Znalazłem więc coś takiego.
  • O REST już coś czytałem, testowałem też sobie operacje na jednej klasie za pomocą Spring Boot i Postman, ale nie chciałem się za to brać na pierwszy projekt... I tak rzuciłem się na głęboką wodę pisząc to. Na kolejny pomyślę :)
  • Postaram się.

Również pozdrawiam.

1 użytkowników online, w tym zalogowanych: 0, gości: 1