Code review projektu :)

Witam

Proszę o code review projektu :) Oczywiście projekt jest w fazie rozwoju...

https://github.com/luxus0/hotel

Nie mam czasu aby ogarnąć całość, trochę drobiazgów

• Zdziwiony jestem dualną architekturą JDBC / JPA. To bywa w "zasłużonych" projektach, ale w nowym do CV? To wklejki z dwóch całości?
• Niejasne zamiary co do klas, np: ma import JPA ale nie używa
  https://github.com/luxus0/hotel/blob/master/src/main/java/hotel_app/hotel/jdbc/model/Reservation.java
• Nadmierne importy, IDE nie proponowało usunąć?
• puste klasy
• gdzieś wydawał mi się brak @Autowired - ale nie jestem springowcem
• klasa, której odpowiedzialności nie umiem rozszyfrować
  https://github.com/luxus0/hotel/blob/master/src/main/java/hotel_app/hotel/jdbc/command/SelectRoomQuery.java
  niby kwerenduje, ale nie daje obiektów, zwraca dżejsony, które już gdzie indziej są produkowane itd...

@e bbb: Najpierw zajrzałem w testy.

Przynajmniej ten "Projekt do CV Na Programistę" wyróżnia się na plus od innych "Projektów do CV Na Programistę" rozbudowanymi testami

package hotel_app.hotel.address.dto;

import org.junit.jupiter.api.Test;

class AddressDtoTest {

    @Test
    void getId() {
    }

    @Test
    void getRegionName() {
    }

    @Test
    void getAdressPostalCode() {
    }

    @Test
    void getCity() {
    }

    @Test
    void getStreet() {
    }

    @Test
    void getAdressHouseNumber() {
    }

    @Test
    void getAdressApartmentNumber() {
    }

    @Test
    void setId() {
    }

    @Test
    void setRegionName() {
    }

    @Test
    void setAdressPostalCode() {
    }

    @Test
    void setCity() {
    }

    @Test
    void setStreet() {
    }

    @Test
    void setAdressHouseNumber() {
    }

    @Test
    void setAdressApartmentNumber() {
    }

    @Test
    void testEquals() {
    }

    @Test
    void canEqual() {
    }

    @Test
    void testHashCode() {
    }

    @Test
    void testToString() {
    }
}

Nie rozumiem za co odpowiada canEqual() ale podejście "nie ufaj nikomu" i testuj 'preimplementowane' hashCode & equals daje do myslenia.

Testowana klasa AddressDTO

package hotel_app.hotel.dto;

public class AddressDTO {

    private String city;
    private String street;
    private Integer postalCode;

    public String getCity() {
        return city;
    }

    public void setCity(String city) {
        this.city = city;
    }

    public String getStreet() {
        return street;
    }

    public void setStreet(String street) {
        this.street = street;
    }

    public Integer getPostalCode() {
        return postalCode;
    }

    public void setPostalCode(Integer postalCode) {
        this.postalCode = postalCode;
    }
}

OK, a teraz pytanie, na czym ma polegać publiczne "review"?

https://github.com/luxus0/hotel/blob/171062808128cfde58bc8c2f098c3e283e583a3d/src/main/java/hotel_app/hotel/service/DeleteReservationDateService.java#L22-L38
Obawiam się, że to nie działa tak, jakbyś oczekiwał.

@crejk:

w tym samym pliku

@AllArgsConstructor
@Service
final class DeleteReservationDateService {

    private final ReservationDateRepository reservationDateRepository;
    private final Logger log = LoggerFactory.getLogger(DeleteReservationDateService.class);

Zaoszczędzamy 3 linie jawnego konstruktora kosztem 2 linii Lomboka, dla mnie strata czytelności

pozdrawiam, zegnam się z wątkiem.

Generalnie jest słabo. Kod jest niechlujny i niepoprawny. Używasz wysokopoziomowego API (Spring Data), które nie rozumiesz jak działa pod spodem. Nie rozumiem też motywacji za tworzeniem serwisu takiego jak DeleteRoomService, który umie usunąć encję na 1001 sposobów.

Przykładowo, tego kodu nie rozumiem:

 public void delete(Room room) {
        roomRepository.delete(room);
        if(roomRepository.findAll().isEmpty())
        {
            log.info("DELETE ROOM: ");
        }
        else
        {
            log.error("DON'T DELETE ROOM");
        }

    }

Porady na przyszłość: spisz sobie funkcje, które ma obsługiwać Twoja aplikacja i dopisz do nich testy. Wywal te logowania. Poczytaj, jaką rolę w aplikacji pełni serwis, a jaką DAO/repozytorium. Poczytaj o architekturze 3-warstwowej, już nie mówię nawet o hexagonie.

Mylace nazwy metod w np. SelectRoomQuery, reszty mi sie nie chce przegladac. Czemu w ogóle piszesz te metody i query do nich?

1. Historia commitów niezbyt ciekawa.
2. Podział na pakiety słaby - np. service taki worek na wszystko - podziel jakoś ze względu na domenę
3. Integer postalCode - zły wybór typu dla kodu pocztowego, np. "00-123" - musisz ucinać myślnik i jeszcze pamiętać, że są zera z przodu. Generalnie kod pocztowy nie jest liczbą mimo, że są w nim cyfry, typ integer jest nielogicznym wyborem
4. gender dałabym enum -no ale może chcesz uwzględnić wszystkie płcie a nie tylko dwie to wtedy ok
5. konsekwentnie stosuj lomboka - w DTO masz ręczne getery i settry
6. AddressController delete powinno zwracać ResponseEntity ze statusem. Bez tego to użytkownik np. puszczjący z postmana, czy frontendowiec nie wiem czy się operacja powiodła czy nie - cofam ten punkt
7. W RoomController masz

private final RoomRepository roomRepository;
    private final ReadRoomService readRoomService;
    private final CreateRoomService createRoomService;
    private final UpdateRoomService updateRoomService;
    private final DeleteRoomService deleteRoomService;

co sugeruje, że odpowiedzialnośc tego controlera jest zbyt duża, pomyśl jak by to podzielić.

8. Konwencja w javie jest by wartości enuma były z dużej litery a jako separator _ a nie camelCase
9. Te jakieś stałe dane w klasie Extra to nie wiem co to ma być i czemu, a sc = new Scanner(System.in); to już mnie zdziwiło kompletnie :D
10. Double priceForNight; - nie stosuj double do cen, to typ do obliczen liczbach z wartościami po przecinku. Tam może ci jakoś dziwnie zwrocić np. 21,99999999999999999. Zamień na BigDecimal
11. Nie czaję z tymi mapperami, stosujesz ModelMapper a potem mapujesz ręcznie, maper powinien to załatwić za ciebie jeśli nazwy pól sa takie same.
12. To jest kompletnie bez sensu:

            roomRepository.updateById(room.getId());
            roomRepository.updateByBeds(room.getBeds());
            roomRepository.updateByPersonNumber(room.getPersonNumber());
            roomRepository.updateByPriceForNight(room.getPriceForNight());
            roomRepository.updateByPrice(room.getPrice());
            roomRepository.updateByAvailable(room.getAvailable());

Po prostu zrób: roomRepository.update(room);

13. Puste testy xD
    ...