Ow u 18

[mrozim78]

Many fixes and refactoring

[SebastianCelejewski]

Kilka moich uwag i refleksji, raczej drobnostki.

1. W przypadku usuwania lub modyfikacji kodu proponuję nie zostawiać kodu zakomentowanego, tylko go usuwać.
2. W warstwie serwisów występują klasy związane z RESTem, np. RestClientErrorMessage, RestClientException itd. Przypuszczam, że albo powinny występować w warstwie webowej, albo trzeba zmienić ich nazwy, jeśli są częścią API warstwy serwisów.
3. RestClientException został zmieniony z RuntimeException na Exception. Celowo użyłem RuntimeException, ponieważ uważam, że checked exceptions to ślepy zaułek ewolucji (konkretnie: komplikuje kod w miejscu wołania metod rzucających checked exceptions, a nie daje żadnych korzyści).
4. RestClientErrorMessage.ALREADY_INIT - proponuję zmienić nazwę stałej, ponieważ jej nazwa nie jest zrozumiała na pierwszy rzut oka. Szczerze mówiąc na drugi rzut oka też nie wiem co może oznaczać.
5. RestClientErrorMessage.USER_ADD_ERROR - taki komunikat błędu nikomu nic nie powie. Jeśli to jest błąd klienta, to trzeba wyraźnie powiedzieć jaki błąd popełnił. Jeśli błąd serwera, to nie należy zwracać w ten sposób informacji o błędzie, tylko rzucić HTTP 500.
6. StringUtils - klasa z jedną metodą, która być może jest już zaimplementowana albo w Apache Commons, albo w Spring Application Framework.
7. UserService - mam obawy co do intuicyjności zwracania obiektu przez metodę, która coś usuwa. Rozumiem, że to jest bardzo przydatne w warstwie webowej do walidacji requesta, ale mam obawy. Bo dostajemy obiekt, który reprezentuje coś, co właśnie zostało usunięte.
8. ResponseDTO - metode buildErrorMessage rzuca wyjątki IllegalAccessException i InstantiationException. Co użytkownik tej metody ma z tymi wyjątkami zrobić? Raczej ich nie obsłuży w żaden sensowny sposób. Proponuję złapać tam wszystkie wyjątki i rzucić jakiś RuntimeException z wyjaśnieniem, że się nie udało i koniec, nic się nie da zrobić.

[SebastianCelejewski]

Jeszcze parę drobnostek:

1. RestClientErrorMessage zawiera niewykorzystane stałe, np. USER_TYPE_IS_MANDATORY, INVALID_PASSWORD, USER_ADD_ERROR. Część z nich ma może przyszłość (np. USER_TYPE_IS_MANDATORY), część pewnie jest do wyrzucenia.
2. Klasy ResponseDTO i ErrorDTO raczej należą do warstwy webowej, a nie do warstwy serwisów.
3. W klasie InitDTO zmieniłbym nazwy pól na number-of-cośtam, np. numberOfPeripheralCommittees itd. Obecne nazwy albo nie bardzo mówią co oznaczają, albo wprowadzają w błąd, np. getPeripheralCommitteeAddress zwraca nie adres, ale liczbę.
4. Uwaga odnośnie formatowania kodu - proponuję zawsze używać automatycznego formatera (plik był w projekcie openpkw-etc.). Fragmenty kodu są niezgodne ze standardem zaproponowanym dla kodu OpenPKW. Np. instrukcje if zawsze mają mieć nawiasy klamrowe itd. (przykład: InitServiceImpl.initDatabase()).
5. Błędy copy-pastingu, np. w klasie InitController logger jest deklarowany dla klasy QrResultController.

[mrozim78]

Zrobiłem kilka poprawek. Co do formatowania kodu jak puściłem wg tych reguł co dałeś to generalnie prawie przerobiło cały projekt. Na razie zrobiłem na dodanych przezemnie klasach.

[pogos]

1. Nasz formater jest zły.... Zapis @RequestMapping(value = "/init", method = RequestMethod.POST, produces = MediaType.APPLICATION_JSON) public ResponseEntity init() jest moim zdaniem mało czytelny(a co gdyby było więcej adnotacji)
2. Klasy typu ErrorDTO, ResponseDTO, RestClientErrorMessage, RestClientException proponuje umieścić we wspólnym pakicie i korzystać z nich we wszystkich serwisach. Być może warto byłoby je scalić w spójne rozwiązanie.
3. Proponuje scalić JpaConfiguration, JpaRepoConfiguration - nic nie wprowadza to, że są to odrębne klasy
4. Jeszcze taki drobiazg - Idea narzeka, że nie są zmienione jej domyślne template dla klas;-)