pull request to show code #1
Conversation
There was a problem hiding this comment.
Dzięki za przesłanie rozwiązania,
Od tego momentu nie rób proszę żadnych zmian, będzie nam łatwiej przeglądać kod :)
Odnośnie PR'a:
- Fajnie, że napisałeś dużo testów 👍
- Ciekawi mnie dlaczego wszystkie commity dodałeś prosto do mastera a MR jest z mastera do
automaticallyGeneratedCodezamiast na odwrót? - Plus za podzielenie pracy na commity, małe i dobrze nazwane
There was a problem hiding this comment.
Dlaczego akurat Readme.txt zamiast Readme.md?
There was a problem hiding this comment.
Traktuję ten pr jako diff po którym można pisać komentarze, ten pr nigdy nie zostanie zmergowany. Z tego powodu nie zwracałem uwagi na to jak nazywa się target oraz source branch. W prawdziwej pracy zrobił bym odwrotnie tak, by po zmergowaniu tego pr zmienił się stan mastera.
Readme.md było by pewnie lepszym wyborem, bo pozwoliło by mi korzystać z markdownu, pewnie jest to też bardziej standardowy wybór. Nie zwróciłem na to uwagi.
| val name: String, | ||
|
|
||
| @Column(nullable = false) | ||
| val price: BigDecimal, |
There was a problem hiding this comment.
Fajnie, że nie użyłeś Double, wiesz dlaczego lepiej używać BigDecimal do operacji na walutach?
There was a problem hiding this comment.
double trzyma w pamięci liczbę w zapisie binarnym, a BigDecimal w postaci zapisu dziesiętnego. Z tego powodu BigDecimal może bez problemu i bez potrzeby zaokrągleń trzymać liczbę taką jak 12.01 zł, podczas gdy double może być jedynie w przybliżeniu równy tej liczbie. Też w obliczeniach finansowych 10 jako podstawa systemu liczbowego jest standardem stosowanym przy zaokrąglaniu i zapisywaniu liczb
| var amountToPay = 0.toBigDecimal() | ||
| for (ticketSeat in seatsAndTicketTypes) { | ||
| val ticketType = ticketSeat.ticketType | ||
| val seat = ticketSeat.seat | ||
| ReservationSeat(reservation, seat, ticketType).let { reservationSeatRepository.save(it) } | ||
| amountToPay += ticketType.price | ||
| } |
There was a problem hiding this comment.
Umiałbyś to zapisać funkcyjnie? (tak by nie używać var) ?
There was a problem hiding this comment.
Można tak
val reservationSeats = seatsAndTicketTypes
.map { (seat, ticketType) ->
ReservationSeat(reservation, seat, ticketType)
.let { reservationSeatRepository.save(it) }
}
val amountToPay = reservationSeats.sumOf { reservationSeat ->
reservationSeat.ticketType.price
}
Rzeczywiście ładniej
| @get:Pattern(regexp="[a-zA-ZAęóąśłżźćńĘÓĄŚŁŻŹĆŃ]*", message="Must contain only letters") | ||
| @get:Size(min = 3, max = 255) | ||
| @get:FirstLetterIsCapital | ||
| var name: String, |
There was a problem hiding this comment.
Nawet próbowałem zrobić by to było val zamiast var, gdy to robiłem nie było jeszcze annotacji @column nad tym name, więc pojawił się błąd bo próbowało zrobić property based access. Lektura internetu doprowadziła mnie do błednego wniosku, że to takie ograniczenie że na @embeddable classes nie da się użyć val w Kotlinie.
| @get:Pattern(regexp="[a-zA-ZAęóąśłżźćńĘÓĄŚŁŻŹĆŃ-]*", message="Must contain only letters and hyphens") | ||
| @get:CapitalizedWordOrTwoCapitalizedWordsWithHyphen | ||
| @get:Size(min = 3, max = 255) | ||
| var surname: String, |
There was a problem hiding this comment.
Podobnie tutaj, dlaczego zdecydowałeś się na var? :)
There was a problem hiding this comment.
dokładnie taki sam błędny powód jak w przypadku name
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "movieScreeningId": 1, | |||
| "name": "Pawel", | |||
There was a problem hiding this comment.
Polskie znaki są obsługiwane, niestety nie wpisałem polskich znaków do tego "scenario" skryptu. Polskie znaki znajdują się w testach
| val lengthInMinutes: Int, | ||
|
|
||
| @Id @GeneratedValue | ||
| val id: Long = 0, |
There was a problem hiding this comment.
hmm, alternatywą by było
val id: Long? = null
To by spowodowało, że id stał by się nullowalny, nie jestem pewien czy to dobrze, z jednej strony zmusza mnie do dodania handlowania nulli, z drugiej strony może wykryję dzięki temu jakiś błąd.
Widziałem argument, by w Javie używać boxed obiektów jako primary key, ponieważ wówczas wiadomo że jak wartość id to null to to na pewno ten null to nie jest prawdziwa wartość z bazy danych, bo w bazie danych id jest nie null.
Znalazłem taki stack overflow
https://stackoverflow.com/questions/49747891/kotlin-jpa-entity-id
ktoś tam argumentuje za używaniem takiej formy jak ja użyłem (albo przynajmniej argumentuje, że też tak można)
Jestem ciekaw jakie jest Twoje zdanie na ten temat
| @Id @GeneratedValue | ||
| val id: Long = 0, | ||
| @Version | ||
| val version: Int = 0, |
There was a problem hiding this comment.
Jest to potrzebne by zrobić optimistic lock tutaj
com/example/ticketbookingapp/service/MovieReservationService.kt:44
Ten lock używam by zapewnić, że nawet gdy kilka użytkowników składa rezerwacje na ten sam seans to nie dojdzie do sytuacji w której nie zajęte miejsce jest otoczone zajętymi miejscami w rzędzie
Version nie ma znaczenia w samej aplikacji, oznacza on ile razy dana rzecz została zmodyfikowana lub optymistycznie zalockowana z LockModeType.OPTIMISTIC_FORCE_INCREMENT
| val name: String, | ||
|
|
||
| @Column(nullable = false) | ||
| val price: BigDecimal, |
There was a problem hiding this comment.
Plusik za BigDecimal. Pytanie, jak ograć temat gdybyśmy chcieli mieć inną walutę jak PLN?
There was a problem hiding this comment.
Dodałbym pole
@Column(nullable = false)
val currency: java.util.Currency,
Hibernate potrafi to zapisać do bazy danych.
Być może tutaj jednak by było potrzeba mieć dla każdego ticket type zbiór (użył bym @ElementCollection) możliwych cen, czyli mieć powiedziane, że bilet studencki kosztuje albo 5 pln albo 1 usd. Wówczas w reservationController klient by mówił w jakiej chce zapłacić walucie i dostawał by w odpowiedzi liczbę lub error message że złą wybrał walutę.
| @Embeddable | ||
| class User( | ||
| @Column(nullable = false) | ||
| @get:Pattern(regexp="[a-zA-ZAęóąśłżźćńĘÓĄŚŁŻŹĆŃ]*", message="Must contain only letters") |
There was a problem hiding this comment.
Ładne, a co jak będzie ktoś miał na imię Mieszko 2 ?:)
There was a problem hiding this comment.
Nie przewidziałem takiego imienia :) Jeśli mogą być spacje i numery to otwiera wiele pytań do tego jakie imiona są poprawne. Na przykład czy imie "P " (dwie spacje po P) jest poprawne? Musiał bym pozadawać kilka takich pytań i na podstawie odpowiedzi zbudował bym bardziej skomplikowany validator
|
|
||
| @Transactional(rollbackFor = [Exception::class], readOnly = true) | ||
| fun getScreeningsInPeriod( | ||
| beginningOfPeriod: Instant, |
There was a problem hiding this comment.
A co w sytuacji, gdy seans jest w różnych strefach czasowy? :)
There was a problem hiding this comment.
Wydaje mi się, że będzie działać dobrze, to znaczy jak użytkownik rest api wpisze jako
beginningOfPeriod 2100-01-01T07:00:00.000%2B05:00
to ten endpoint otrzyma instant reprezentujący
2100-01-01T02:00:00.000%2B00:00
(albo inaczej reprezentujący pewien Unix time)
Innymi słowy nie interesuje tego endpointu w jakiej strefie czasowej lubi reprezentować czas użytkownik.
Zadbałem również o to by w bazie danych zapisywały się Instanty w UTC wpisując w properties
spring.jpa.properties.hibernate.jdbc.time_zone=UTC
| beginningOfPeriod: Instant, | ||
| endOfPeriod: Instant, | ||
| offset: Int, | ||
| limit: Int |
| ) | ||
| ) | ||
|
|
||
| val list: List<MovieScreening> = run { |
There was a problem hiding this comment.
Ta logika zapytań to raczej działka repozytorium..
|
|
||
| @GetMapping("/screening") | ||
| @Transactional(rollbackFor = [Exception::class], readOnly = true) | ||
| fun getScreeningInfo(dto: ScreeningInfoRequestDto): MovieScreeningAndRoomDto { |
There was a problem hiding this comment.
Trochę już późno :) Ale czy ja dobrze rozumiem, że w get przesyłasz payload z informacją o id screeningu?
There was a problem hiding this comment.
Tak. Nie jestem pewien czy o to tutaj chodzi, natomiast można to by uprościć pisząc argument
@RequestParam screeningId: Long
i wyrzucając klasę ScreeningInfoRequestDto
| path = ["/api"], | ||
| produces = ["application/json"] | ||
| ) | ||
| // cors would be changed in final app |
There was a problem hiding this comment.
👍 Są jeszcze inne sposoby poradzenia sobie z tym, można globalnie to ograć.
|
|
||
| @ConfigurationProperties(prefix = "custom") | ||
| data class CustomConfigProperties( | ||
| val minimalTimeBetweenBookingAndScreeningStart: Int |
There was a problem hiding this comment.
Nie wiadomo, czy ten czas jest w minutach czy w sekundach, jak byś to poprawił :) ?
There was a problem hiding this comment.
Jedno rozwiązanie to zmiana nazwy na
minimalTimeBetweenBookingAndScreeningStartInMinutes
Lepsze rozwiązanie to by było zmienić typ tego na Duration. Wówczas mogę w properties napisać
custom.minimalTimeBetweenBookingAndScreeningStart=15m
i to tam wybieram czy to są sekundy czy minuty
| initializeScreening(room3, movie1, 19) | ||
|
|
||
| // to make sure ids of movie screenings are accessible | ||
| entityManager.flush() |
There was a problem hiding this comment.
Wydaje mi się, że to nie byłoby konieczne, jeżeli metody initialize* zwracałyby obiekt z metody save
There was a problem hiding this comment.
Korzystam z metody save() z JpaRepository i te metody initialize* zwracają wynik tej metody.
Nie zdawałem sobie sprawy, że metoda save() zwraca obiekty z ustawionym id, wiedziałem że może być z tym problem jak się używa entityManager.persist i stąd się zabezpieczyłem.
Swoją drogą, nawet jakby metoda save() zwracała obiekty z nie ustawionym id to i tak to flush() tutaj i w klasie DatabaseInitializationForTest nie było by potrzebne, ponieważ nie korzystam z wygenerowanego id, zamiast tego korzystam z referencji do java obiektów. Kiedyś odnosiłem się do id w tutaj poniżej tego flush()
|
|
||
| @Entity | ||
| class MovieScreening( | ||
| @ManyToOne(fetch = FetchType.LAZY, optional = false) |
There was a problem hiding this comment.
Mam wrażenie, że wszędzie zrobiłeś leniwe ładowanie (FetchType.LAZY), czy w tej sytuacji to dobry wybór? Co nam to daje?
There was a problem hiding this comment.
W książce w z której uczyłem się Hibernate autor narzekał, że eager fetch type w @manytoone to zły default, że jego zdaniem defaultem powinno być lazy, stąd uczyniłem lazy defaultem. Można by sobie wyobrazić egzotyczną bazę danych w której z powodu eager loading w @manytoone załadowało by się przez przypadek całą bazę danych (bo obiekt ma dwie propertki oznaczone przez @manytoone, więc ładując obiekt ładuję dwa inne obiekty, każdy z nich może znowu ma dwa @manytoone i tak dalej)
Możliwe, że tutaj to lazy loading jest szkodliwe dla performance. Muszę doczytać czym się kierować wybierając między lazy a eager loadingiem. Wydaje mi się, że na przykład w przypadku propertki
val ticketType: TicketType
w ReservationSeat eager może być lepszy, bo gdy ładujemy reservation seat to zwykle chcemy też mieć ticket type.
| val movieScreening: MovieScreening, | ||
|
|
||
| @Column(nullable = false) | ||
| val expirationDate: Instant, |
There was a problem hiding this comment.
Powiesz czemu Instant a nie np. LocalDateTime? Zdradzę, że mi akurat takie podejście się podoba, ale ciekaw jestem jakie Ty masz powody ;)
There was a problem hiding this comment.
Chciałem uniknąć problemów z strefami czasowymi.
Instant odpowiada pewnemu unix time (albo alternatywnie czasowi w utc), nie interesują go takie udziwnienia jak strefy czasowe.
Gdybym miał w memory obekty typu LocalDateTime to bym się musiał zastanawiać której strefie czasowej one odpowiadają. Wówczas bym przyjął, że odpowiada strefie czasowej utc, ale to wówczas bym równie dobrze (a właściwie lepiej) mógł używać Instant. A jakbym przyjął, że LocalDateTime odpowiadają strefie czasowej kraju w którym znajduje się kino to myślę, że bym otworzył puszkę Pandory, np związaną z handlowaniem zmian strefy czasowej, albo handlowaniem tego że część kin jest w jednym kraju a część w innym.
W settingsach mam linię
spring.jpa.properties.hibernate.jdbc.time_zone=UTC
by te Instanty mi się zapisywały w utc w bazie danych
No description provided.