ticket-booking code review #1
Conversation
|
|
||
|
|
||
| ### Additional assumptions | ||
| - Requested time interval for point 1. of the scenario can be at max 1 week. |
There was a problem hiding this comment.
Żeby ograniczyć liczbę jednorazowo pobieranych rekordów. Bez tego użytkownik mógłby pobrać seanse od teraz na kilka miesięcy w przód. Ograniczenie wyszukiwania do jednego tygodnia może być w pewnym sensie paginacją. Dodatkowo uznałem, że użytkownik szukając seansu rzadko potrzebowały dłuższego przedziału.
There was a problem hiding this comment.
Super, cieszę się, że to zauważyłeś!
| Example output: | ||
| ```agsl | ||
| 1. Fetch movie screenings that start between 15:20 and 18:00 on 05.04.2023: | ||
| [{"movieTitle":"Django","screeningTimes":[{"screeningId":"4941fe3f-611b-48a2-b31d-ed2fc551069f","start":"2023-04-05T16:30:00","end":"2023-04-05T18:00:00"}]},{"movieTitle":"Hateful 8","screeningTimes":[{"screeningId":"cd174523-4630-4f36-9611-5a78d663e15a","start":"2023-04-05T15:30:00","end":"2023-04-05T17:00:00"},{"screeningId":"fd612a91-db41-4bc3-a794-f8c1a3d8e1d2","start":"2023-04-05T17:30:00","end":"2023-04-05T19:00:00"}]},{"movieTitle":"Pulp Fiction","screeningTimes":[{"screeningId":"a80be8de-0c61-4e0a-a3e5-95b055845c18","start":"2023-04-05T16:00:00","end":"2023-04-05T17:30:00"}]}] |
There was a problem hiding this comment.
Fajnie byłoby to przepuścić przez jakiś formater (jq?).
There was a problem hiding this comment.
Rzeczywiście byłoby czytelniej, sorka :)
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-validation</artifactId> | ||
| <version>RELEASE</version> |
There was a problem hiding this comment.
Ups, miało być bez wersji, powinna być z parenta. LATEST i RELEASE chyba nie działają dla wersji pluginów ale w dependency już tak.
| @Configuration | ||
| public class ServicesConfiguration { | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
Czemu tworzymy tak, a nie przez adnotację nad serwisem?
There was a problem hiding this comment.
Oczywiście można dodać adnotację na poziomie serwisu domenowego, ale chciałem, żeby domena nie zawierała springowych adnotacji.
|
|
||
| @ExceptionHandler | ||
| public ResponseEntity<ErrorMessage> invalidScreeningTimeIntervalExceptionHandler(InvalidScreeningTimeIntervalException exception) { | ||
| log.error("Exception thrown InvalidScreeningTimeIntervalException: {}", exception.getMessage(), exception); |
There was a problem hiding this comment.
Kiedy dobrze jest logować na error a kiedy na warn?
There was a problem hiding this comment.
Error do błędów, które uniemożliwiają poprawne funkcjonowanie aplikacji, natomiast warn gdy coś jest nie tak, ale aplikacja działa prawidłowo. Tutaj można by to pozmieniać na warny.
There was a problem hiding this comment.
Dla mnie fajną regułą kciuka jest czy człowiek (administrator?) musi reagować - jak tak to error, jak nie to warn.
| public boolean isValid(Object object, ConstraintValidatorContext constraintValidatorContext) { | ||
| try { | ||
| Field startField = object.getClass().getField(startFieldName); | ||
| startField.setAccessible(true); |
There was a problem hiding this comment.
A co tu się wydarzyło? To brzmi jak bardzo brzydka rzecz. Nie wystarczyłoby odpowiednio użyć genericów/castowania?
There was a problem hiding this comment.
Żeby móc założyć adnotacje @ValidLocalDateTimeRange na dowolnym obiekcie musiałem skorzystać z refleksji i ustawić setAccessible(true) żeby odczytać niezbędne wartości z prywatnych pól. Żeby pozbyć się refleksji można by ograniczyć adnotację do konkretnej klasy ScreeningEntity, wtedy po prostu można skorzystać z getterów.
There was a problem hiding this comment.
Można by to też pewnie obejść tworząc klasę DateTimeRange i używając jej wszędzie tam gdzie potrzebujesz zakresu dat.
|
|
||
| private Reservation createReservation(RequestedReservationDto requestedReservationDto, Screening screening) { | ||
| List<Ticket> tickets = getTickets(requestedReservationDto); | ||
| log.debug("Creating reservation for tickets: {}", tickets); |
There was a problem hiding this comment.
Zmiany stanu fajnie jednak logować na info.
|
|
||
| private void validateParameters(String title, String description) { | ||
| if (title.isBlank()) { | ||
| throw new IllegalArgumentException("Movie title must not be empty"); |
There was a problem hiding this comment.
Tyle fajnych wyjątków a tu nagle takie cudo :)
There was a problem hiding this comment.
Przy takiej podstawowej walidacji rzucałem IllegalArgumentException i zastanawiałem się nawet czy nie zrobić wyjątku domenowego. Cóż... trzeba było mi to jednak zrobić :)
|
|
||
| private List<MovieScreeningsDto> sortByMovieTitle(List<MovieScreeningsDto> movieScreeningsDtos) { | ||
| return movieScreeningsDtos.stream() | ||
| .sorted(Comparator.comparing(MovieScreeningsDto::getMovieTitle)) |
There was a problem hiding this comment.
- Jak to się zachowa dla polskich znaków?
- Czemu nie posortować w bazie?
There was a problem hiding this comment.
- Aj, rzeczywiście z sortowaniem tytułów z polskimi znakami robi się problem. Tak to jest jak testuje się na filmach Tarantino.
- Bo chciałem żeby tego typu wymagania biznesowe nie opuszczały domeny. Jeśli sortowanie po stronie aplikacji byłoby niewydajne można by to mieć posortowane w bazie, ale mogła by to być przedwczesna optymalizacja.
| @@ -0,0 +1,6 @@ | |||
| package com.elmc.booking.domain.screening; | |||
|
|
|||
| public enum SeatStatus { | |||
There was a problem hiding this comment.
Jeśli ten enum nie wycieka (bo masz ładne isBooked w Seat) to może to powinien być prywatny enum tamtej klasy?
There was a problem hiding this comment.
Z tego enuma korzystam w kilku miejscach w kodzie. Mógłbym go zrobić prywatnym enumem, ale wymagałoby to kilku zmian.
No description provided.