Wątek przeniesiony 2023-07-05 12:08 z Java przez Riddle.
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:
com.example.carrentalsystem.Controllers powinno być com.example.carrentalsystem.controllers
E, np. ERole. Zazwyczaj pisze się RoleEnum, ewentualnie Roles (chociaż to dużo rzadsze).getBrand_Name() -> getBrandName())Design:
Rental.price jest Integerem, a nie Longiem, przy czym w samym kodzie jest sprawdzenie, czy przy liczeniu nowej ceny licznik się nie przekręciLocalDate.now() zamiast oddzielnego serwisu do dat - takie coś utrudnia testowalność.Kod:
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.
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.
existsBy, czasem pobierając listę przez findAll i sprawdzając, czy są elementy, a czasem przez countBy.findById, a np. CarRepository ma już getCarById który robi praktycznie to samoMuszę przyznać, że jestem na tyle stary, że logiki tego nie ogarniam:
if(brandRepository.countByName(brand.getName()) == 1){
brandRepository.deleteByName(brand.getName());
}
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
@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/typZasobu/{idZasobu} dla konkretnej instancji i /typZasobu z ewentualnymi query parameters /typZasobu?kolor=zielony dla listy zasobów. .../list jest nieintuicyjne. @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
@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.@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.