Ocena aplikacji do wypożyczania samochodów

Cześć wszystkim, od jakiegoś czasu tworzę aplikacje webową opartą o Springa związaną z wynajmem aut. Jest to projekt do portfolio, który ma dość prosty mechanizm działania.
Jako, że piszę ten kod sam, to w zasadzie nie mam skąd dowiedzieć się czy to co tworzę ma jakkolwiek ręce i nogi, zatem chciałbym poprosić osoby bardziej doświadczone o konstruktywną krytykę, wskazówki i rady :) Z góry dziękuję.

Link do repozytorium - https://github.com/Mr-Victor16/car-rental-system-spring

Jeśli chcesz zajmować się programowaniem profesjonalnie, to commity również trzeba pisać po angielsku. Ogólnie angielski jest językiem programistów. Tak samo readme. O ile kierujesz przekaz do programistów (a nie do użytkowników końcowych), to należy założyć, że odbiorca zna na pewno angielski, a polski niekoniecznie.

LukeJL napisał(a):

Jeśli chcesz zajmować się programowaniem profesjonalnie, to commity również trzeba pisać po angielsku. Ogólnie angielski jest językiem programistów. Tak samo readme. O ile kierujesz przekaz do programistów (a nie do użytkowników końcowych), to należy założyć, że odbiorca zna na pewno angielski, a polski niekoniecznie.

Myślałem nad tym, aczkolwiek nie wiem czy nie będzie to dość dziwne, gdy teraz od pewnego momentu wszystko będzie po angielsku, a wstecz jeszcze będą commity po polsku.

lepiej późno niż wcale ;)

Chcesz review jedynie do Springa, czy frontend także Cię interesuje?

Xarviel napisał(a):

Chcesz review jedynie do Springa, czy frontend także Cię interesuje?

Najlepiej jednego i drugiego, lecz była to część forum poświęcona głównie Javie, zatem na razie nawet nie prosiłem o więcej niż backend

Nazewnictwo:

1. Nazwy pakietów z dużej litery - tego ogólnie nie robimy. Np. zamiast com.example.carrentalsystem.Controllers powinno być com.example.carrentalsystem.controllers
2. Nazywanie enumów od E, np. ERole. Zazwyczaj pisze się RoleEnum, ewentualnie Roles (chociaż to dużo rzadsze).
3. Korzystaj z camelCase i nie używaj podkreślnika w nazwach (np. getBrand_Name() -> getBrandName())

Design:

1. Brak warstwy serwisów, cała logika siedzi w kontrolerach
2. Brak testów jednostkowych
3. Z jakiegoś powodu Rental.price jest Integerem, a nie Longiem, przy czym w samym kodzie jest sprawdzenie, czy przy liczeniu nowej ceny licznik się nie przekręci
4. Używasz LocalDate.now() zamiast oddzielnego serwisu do dat - takie coś utrudnia testowalność.

Kod:

1. Dziwne nullchecki. Np.

Brand brand;
if (brandRepository.findByName(carRequest.getBrand()) != null) 
  brand = brandRepository.findByName(carRequest.getBrand);
} else { 
  brand = brandRepository.save(new Brand(carRequest.getBrand()));
}

Oznacza to, że zawołasz bazę danych dwa razy - raz, żeby sprawdzić, czy dany brand istnieje, drugi - żeby przypisać. Nie ma to większego sensu i można napisać to tak:

Brand brand = brandRepository.findByName(carRequest.getBrand());
if (brand == null) {
  brand = brandRepository.save(new Brand(carRequest.getBrand())); 
}

Albo jeszcze lepiej - użyć optionala:

Brand brand = Optional.ofNullable(brandRepository.findByName(carRequest.getBrand()))
  .orElseGet(() -> brandRepository.save(new Brand(carRequest.getBrand()));

Ewentualnie przerobić BrandRepository.findByName(String) tak, żeby zwracało Optional<Brand>. Tego typu rzeczy jest trochę w kodzie - to tylko jeden z przykładów.

1. Ogólnie zacznij wydzielać kod do oddzielnych metod lub klas (to się łączy z poprzednim komentarzem o kontrolerach). Na przykładzie RentalController.changeRentalInformation(EditCarRentalRequest):

Jest:

public ResponseEntity<?> changeRentalInformation(@RequestBody @Valid EditCarRentalRequest request){
  if(rentalRepository.existsById(request.getRentId())){
    Rental rental = rentalRepository.getReferenceById(request.getRentId());
    rental.setPrice(Math.toIntExact((ChronoUnit.DAYS.between(request.getStartDate(), request.getEndDate())+1) * rental.getCar().getPrice()));
    rental.setStartDate(request.getStartDate());
    rental.setEndDate(request.getEndDate());
    rentalRepository.save(rental);
      
    return new ResponseEntity<>("Rent details changed", HttpStatus.OK);
  }

    return new ResponseEntity<>("No rental found", HttpStatus.NOT_FOUND);
  }
}

Napisałbym to mniej-więcej tak:

public ResponseEntity<?> changeRentalInformation(@RequestBody @Valid EditCarRentalRequest request){
  if(rentalRepository.existsById(request.getRentId())){
    Rental rental = rentalRepository.getReferenceById(request.getRentId());
    updateEntity(rental, request); 
    rentalRepository.save(rental);
      
    return new ResponseEntity<>("Rent details changed", HttpStatus.OK);
  }

    return new ResponseEntity<>("No rental found", HttpStatus.NOT_FOUND);
  }
}

private void updateEntity(Rental entity, EditCarRentalRequest request) {
    entity.setPrice(Math.toIntExact((ChronoUnit.DAYS.between(request.getStartDate(), request.getEndDate())+1) * rental.getCar().getPrice()));
    entity.setStartDate(request.getStartDate());
    entity.setEndDate(request.getEndDate());
}

Oczywiście w tym przypadku może nie widać zbytnich korzyści z tego, ale już jeśli zrobisz coś podobnego przy CarController.editCar(EditCarRequest) to różnica będzie duża.

1. Brak spójności - czasem sprawdzasz, czy encja istnieje w bazie danych przez existsBy, czasem pobierając listę przez findAll i sprawdzając, czy są elementy, a czasem przez countBy.
2. Brak spójności x2 - tj. w niektórych repozytoriach używasz findById, a np. CarRepository ma już getCarById który robi praktycznie to samo

Muszę przyznać, że jestem na tyle stary, że logiki tego nie ogarniam:

if(brandRepository.countByName(brand.getName()) == 1){
    brandRepository.deleteByName(brand.getName());
}

https://github.com/Mr-Victor16/car-rental-system-spring/blob/cffc1aa1475993c189def0c469111104c577ebea/src/main/java/com/example/carrentalsystem/Controllers/CarController.java#L116

szczególne w kontekście tego:
https://github.com/Mr-Victor16/car-rental-system-spring/blob/cffc1aa1475993c189def0c469111104c577ebea/src/main/java/com/example/carrentalsystem/Controllers/CarController.java#L54

(+ oczywiście cała reszta podobnej logiki z CarControllera)

Pobieżne bardzo czytanie, to co mi się w oczy rzuciło

1. Testy. Co testują twoje testy, skoro nie ma w nich asercji?
2. Model Używasz Lomboka, dlaczego w takim razie oprócz automatycznie generowanego konstruktora bezargumentowego dopisujesz ręcznie konstruktory z argumentami?
3. Kontroler @CrossOrigin(origins = "*", maxAge = 3600) mniejszy problem, to ustawienie wartości parametrów - pierwszy jest niepotrzebny, bo ustawia wartość domyślną. Większy problem, to wprowadzenie taką wartością domyślną podatności CSRF
4. Błędne schematy ścieżek REST np. tutaj. Typowe i intuicyjne mapowanie CRUD'a to /typZasobu/{idZasobu} dla konkretnej instancji i /typZasobu z ewentualnymi query parameters /typZasobu?kolor=zielony dla listy zasobów. .../list jest nieintuicyjne.
5. To samo miejsce:

    @GetMapping("list")
    public ResponseEntity<?> getRentalStatusList(){
        if(rentalStatusRepository.count() > 0){
            return ResponseEntity.ok(rentalStatusRepository.findAll());
        }

        return new ResponseEntity<>("No rental statuses found", HttpStatus.NOT_FOUND);
    }

zamiast

    @GetMapping("list")
    public ResponseEntity<RentalStatus> getRentalStatusList(){
            return ResponseEntity.ok(rentalStatusRepository.findAll());
    }

Co jest krótsze, wiadomo co zwróci i pusta lista jest bardziej intuicyjna i prostsza do obsłużenia dla konsumenta tego API

1. W miejscach gdzie masz zwrócić obiekt o konkretnym id, jest ten sam babol. Jeżeli nie znaleziono jakiegoś obiektu, to z REST zwracasz null + status 404
2. @PostMapping("status/{statusID}/rental/{id}") POST nie powinien definiować id wstawianego obiektu (ostatni parametr), jeżeli chcesz robić update, to masz PUT https://restfulapi.net/idempotent-rest-apis/

• @Order(2) w testach? No na pewno nie.
• Testy są tragicznej jakości
• Cała apka wygląda jak typowy CRUD, nie ma tu za wiele do oceny

@wartek01: Wszystkie punkty z nazewnictwa odhaczone. Teraz skupię się na testach jednostkowych.

@jarekr000000: Miałem odpisać jako komentarz, lecz już jest osiągnięty limit komentarzy do odpowiedzi - Już w niektórych sytuacjach zastanawiam się, czy na pewno powinienem coś usuwać, więc to co mówisz z pewnością przyjdzie dość szybko.

@piotrpo Poprawiłem ścieżki REST, zmieniłem metody HTTP na bardziej dopasowane do zastosowania oraz poprawiłem zwracanie według wskazówek. O pozostałych punktach oczywiście też pamiętam i staram się stopniowo wszystko przerabiać, tak jak być powinno.

@Riddle Tak jak wspomniałem już wyżej, w niedługim czasie zajmę się poprawieniem, a w zasadzie napisaniem prawdziwych testów.

W międzyczasie pojawił mi się również problem przy zwracaniu listy wynajmów.
Gdy metoda wygląda następująco, to zwraca mi listę lecz statusHistory będzie puste:

    @GetMapping
    @PreAuthorize("hasRole('ADMIN')")
    public ResponseEntity<?> getAllRentals(){
        return ResponseEntity.ok(rentalService.findAll());
    }

Lecz, gdy dodam np. wypisanie czegoś na konsolę, to statusHistory będzie już taki jak należy, czyli nie będzie pusto:

    @GetMapping
    @PreAuthorize("hasRole('ADMIN')")
    public ResponseEntity<?> getAllRentals(){
        List<Rental> list = rentalService.findAll();
        System.out.println(list.get(0).getStatusHistory().get(0).getStatusAfterChange().getName());
        return ResponseEntity.ok(rentalService.findAll());
    }

Odnośnik do dokładnej linii kodu w repozytorium https://github.com/Mr-Victor16/car-rental-system-spring/blob/main/src/main/java/com/example/carrentalsystem/controllers/RentalsController.java#L25

Ja spojrzałem tylko na controller. Za dużo logiki. I tyle. Nie wiem jak dalej.