Merge all to master by mateuszkp96 · Pull Request #1 · mateuszkp96/TicketBookingApp

mateuszkp96 · 2019-07-05T10:16:49Z

Screening room seats arrangement

…ds added

… response 400

MateuszRasinski

Na plus: zrealizowano funkcjonalność rezerwacji wraz z walidacją miejsc. Dobry interfejs, zmapowane wyjątki, pieniądze w BigDecimal. Widać, że lubisz się i dobrze sobie radzisz z SQLem.
Na minus: brak testu przynajmniej happy path (chociaż jest w bashu, więc tyle dobrze). Pakiety dzielone horyzontalnie (po warstwach technicznych) zamiast wertykalnie (po domenie biznesowej, funkcjonalnościach). Uboga domena, wszystko dzieje się proceduralnie i głównie po stronie bazy danych. Wszystko oprócz walidacji miejsc, która jest bardzo skomplikowana i nieczytelna.

Proszę, nie poprawiaj kodu, ale odnieś się do komentarzy. Dzięki!

MateuszRasinski · 2019-07-10T12:06:21Z

src/main/java/com/mkopec/ticketbookingapp/controller/ReservationController.java

+    @PostMapping
+    public ShortReservationDTO postReservation(@RequestBody ReservationPostDTO postDTO) {
+        Reservation reservation = mapper.toReservation(postDTO);
+        return mapper.toShortReservationDTO(service.saveReservation(reservation));


nie jestem fanem mapperów, ale spoko. Generalnie do service'u mogłoby przyjść DTO i mogłoby być zwrócone DTO i byłoby OK

MateuszRasinski · 2019-07-10T12:07:50Z

src/main/java/com/mkopec/ticketbookingapp/domain/Movie.java

+    private String director;
+
+    @Basic
+    private java.time.LocalTime length;


import? i po co ten @Basic ?

Nie wiem o który import tutaj chodzi, prawdopodobnie o import static. Słyszałem, że dobrą praktyką jest import statyczny tych stałych (zwłaszcza gdy w jednej adnotacji może ich występować więcej). A adnotacja @basic to chyba pozostałość po wcześniejszych wersjach springa, ogólnie wzorowałem się na: https://www.baeldung.com/hibernate-date-time a tam pola oznaczone są tą adnotacją. W springu 5 działa dobrze już bez tej adnotacji.

Chodziło mi o to, że tą klasę LocalTime można zaimportować i nie mieć kropeczek wtedy (java.time można się pozbyć)

Racja, mój błąd

MateuszRasinski · 2019-07-10T12:08:20Z

src/main/java/com/mkopec/ticketbookingapp/domain/Reservation.java

+
+import lombok.Data;
+
+import javax.persistence.*;


importy z gwiazdkami nie są preferowane - dobre IDE i tak ci je schowa, a jak chcesz zobaczyć jaka klasa jest importowana to tak nie zobaczysz :(

Nie wiedziałem o tym, ale to kwestia ustawień IDE. Teraz już będę pamiętał

MateuszRasinski · 2019-07-10T12:09:50Z

src/main/java/com/mkopec/ticketbookingapp/domain/RoomSeat.java

+    private Room room;
+
+    @Column(name = "is_on_edge")
+    private Boolean isOnEdge;


ciekawa koncepcja. A bez tego by się dało? I bez dodatkowej tabelki złączeniowej?

Przyjąłem dodatkowe założenie, że miejsce na skraju może być zawsze zarezerwowane( Przy samym założeniu, że nie można zostawić jednego miejsca wolnego byłby problem z rezerwacją tego miejsca na skraju [ REZERWOWANE, WOLNE, ZAJĘTE, .... ]. Wtedy nie musimy sprawdzać sąsiednich miejsc. To pozwala również na układ sali taki, że np. na środku sali może być przejście, wtedy mamy kolejne miejsca na skraju. ) Stąd ta zmienna isOnEdge. Z kolei obeszło by się bez tabelki złączeniowej, po prostu wrzucić numer rzędu i siedzenia do tej klasy.

MateuszRasinski · 2019-07-10T12:10:35Z

src/main/java/com/mkopec/ticketbookingapp/domain/TicketType.java

+
+    private String typeName;
+
+    private BigDecimal price;


plus za BigDecimal

MateuszRasinski · 2019-07-10T12:31:17Z

src/main/java/com/mkopec/ticketbookingapp/service/SeatValidationService.java

+            }
+        });
+        return seats;
+    }


dlaczego to nie jest w jakiejś klasie? abstrakcji zamykającej salę kinową?

Tak, tutaj można by to opakować w abstrakcje, jakiś typ generyczny implementujący interfejs do pobierania danych zamiast RoomSeat

MateuszRasinski · 2019-07-10T12:32:12Z

src/main/java/com/mkopec/ticketbookingapp/service/SeatValidationService.java

+        }
+
+        return true;
+    }


ta klasa ma tyle ifów i switchów, że nigdy nie chciałbyś takiej reviewować / utrzymywać.

Sporo ifów się zrobiło z powodu iterowania po liście po sąsiednich miejscach (nie zawsze występują w sąsiedztwie dwa na lewo i dwa na prawo jak w ogólnym przypadku). Nie zastosowałem listIterator, ponieważ po wywołaniu next() lub previous() zmienia się index wskazywanego elementu, a tutaj było to niepożądane.

MateuszRasinski · 2019-07-10T12:33:46Z

src/main/java/com/mkopec/ticketbookingapp/service/SeatValidationService.java

+
+    private boolean hasNext(List<SeatModel> neighbourhood, int index) {
+        return index + 1 < neighbourhood.size();
+    }


te 4 metody wyglądają jakby zawsze dotyczyły tego samego. Może można by je zamknąć w jakiejś jednej klasie? Jest coś takiego jak programowanie obiektowe.

Racja, mogłem to opakować, ewentualnie zamienić na abstrakcję z typem generycznym

MateuszRasinski · 2019-07-10T12:35:56Z

src/test/java/com/mkopec/ticketbookingapp/service/SeatValidationServiceTest.java

+import static org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Enclosed.class)
+public class SeatValidationServiceTest {


jeżeli miałbyś napisać jeden test, to jaki byłby to test? Test tego, że osoba wyświetla filmy, wybiera sens, dokonuje rezerwacji i system tą rezerwację przyjmuje; czy testy walidacji zajętych miejsc?

Przyznaję, że brakuje tutaj testów use casów (same w sobie zawierałyby kilka testów rezerwacji miejsc). A test napisany z powodu dużej ilości kombinacji ustawień siedzeń, ciężkie do sprawdzenia w sposób ręczny.
Więc jeśli miałbym wybierać to test use case - sprawdza więcej elementów.

MateuszRasinski · 2019-07-10T12:36:20Z

src/test/java/com/mkopec/ticketbookingapp/service/SeatValidationServiceTest.java

+
+        @Test
+        public void test_validate_good() {
+            assertTrue(service.validate(reserved, occupied, all));


trochę mało czytelne te testy

Tutaj kwestia nazewnictwa - te metody mogłyby nazywać się lepiej oraz brakuje wyrównania parametrów w metodach

= and others added 30 commits June 29, 2019 18:06

Seat and Room domain models added

5ce2926

Movie and screening domain models added

9de1ac6

Reservation and ticket domain models added

e8a260b

application.properties for h2 database set

5902980

Domain model schema sql script added

6883415

Sql script with initial data added

582402e

Screening repository added

cadc4a1

ScreeningService added

6b61c31

MapStruct dependency to build.gradle added

0720dd9

ShortScreeningDTO added

83f7ab2

ScreeningMapper added

7536fd7

ScreeningController added

595bae1

RoomSeatRepository and method for available seats added

7d57e4d

GET screening details and screening DTO added

d575ebf

DTOs for post reservation and mappers added

d8d8083

Reservation controller and service added

c5eaef3

isOnEdge attribute to seat domain model added

0c5b331

Find all seats in row and find occupied seats in row repository metho…

49c36a8

…ds added

ReservationValidationService added with parameterized test

af7e011

TicketType service added to calculate payment for tickets

d89cb63

Validation in save method of ReservationService added

1811959

Cascade persist in reservation for Tickets added

4339486

Exceptions in services changed to ArgumentNotValidException with Http…

a584589

… response 400

build jar added

ab5b7f0

execution permission for gradlew added

1596c81

expected exception changed in test of seatvalidationService

29fbdf3

Build and run shell script added

d42503b

Run use cases shell script added

9a2f5a8

run use cases shell script without jq json formating added

b921619

Create README.md

cb5f3c7

MateuszRasinski reviewed Jul 10, 2019

View reviewed changes

Conversation

mateuszkp96 commented Jul 5, 2019

Uh oh!

MateuszRasinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants