Full changes log merge from beginning of repo #1

Diamekod0221 · 2023-11-01T23:22:32Z

Pull request for code review.

… List.

…ces.

…alidator

# Conflicts: # src/main/java/com/ramcel/cinema/reservation/Application.java # src/main/java/com/ramcel/cinema/reservation/functionalities/controllers/TicketController.java # src/main/java/com/ramcel/cinema/reservation/functionalities/ticket/validators/TicketValidator.java # src/main/resources/application.properties # src/test/java/com/ramcel/cinema/reservation/controller/TicketControllerTest.java

philemone

Cześć,
dzięki za przesłanie zadania - nie rób proszę już żadnych zmian pomoże nam to łatwiej przeglądać.

Od siebie dodałem kilka komentarzy 🙂

philemone · 2023-11-06T08:59:05Z

bash_scripts/db_setup.bash

+CONTAINER_NAME="mysql-container-nussknacker"
+
+DB_USER="cinema-app"
+DB_PASSWORD="EPqKjMPYhwJIeWKVRVQ7"


Hasło do bazy danych zapisane jako plain text wygląda groźnie 😨 Czy hasło musi być w repozytorium? Jak mógłbyś to zapisać by nie było zapisane w git'cie ani widoczne dla kogoś kto tego hasła nie ma? 🤔

To prawda, że trochę się zastanawiałem, czy to tak wrzucać, ale to nie jest taki "prawdziwy" secret. Normalnie jest kilka rozwiązań, najbardziej popularne jakie znam, to (przy używaniu apki w dockerze, a ja tego nie zrobiłem), wrzucić secret jako env variable dla kontenera i ustawić w springu jako enviornment variable, np przy użyciu Property. Spring udostępnia też kilka bibliotek takich, jak jasypt czy spring vault pomocnych przy używaniu secretów.

philemone · 2023-11-06T09:06:48Z

how_to_run.txt

+Please install docker, java 21, maven.
+
+Go to /bash_scripts.
+First run the db_setup.bash script - it should start a docker container with mysql 8.0 running. The container is


Fajnie, że napisałeś skrypt :) a dlaczego nie zdecydowałeś się na użycie docker-compose?

mvn spring-boot::run to jest moje preferowane podejście przy takich szybkich odpaleniach, nie trzeba nic konfigurować na classpath, zwłaszcza jeśli chodzi o zewnętrzne pliki. Budowanie jara i docker-compose czasem wymagają jakiegoś grzebania, a nie bardzo miałem na to czas. Myślę, że w tym przypadku są to równie eleganckie rozwiązania, maven odpala się zwykle szybciej niż docker i bez problemu.

philemone · 2023-11-06T09:07:33Z

pom.xml

+        </dependency>
+		<dependency>
+			<groupId>com.h2database</groupId>
+			<artifactId>h2</artifactId>


Fajnie, że używasz in-memory db - h2 👍

philemone · 2023-11-06T09:10:00Z

readme.txt

@@ -0,0 +1,69 @@
+    About the app:


Dlaczego wybrałeś README jako readme.txt a nie readme.md? Jaka jest różnica i co by Tobie (i użytkownikom) to dało? :)
Chyba, że celowo zamysł był na taki low-tech vibe 🤓

Lubię .txt :) Różnica jest taka, że w markdownie można dodać lepszy formatting i np hyperlinki. Trochę ładniej możnaby zrobić.

philemone · 2023-11-06T09:14:43Z

bash_scripts/db_setup.bash

+for SCRIPT in "$SQL_SCRIPTS_DIR"/*.sql; do
+    SCRIPT_NAME=$(basename "$SCRIPT")


Znasz może jakieś narzędzia/biblioteki, które pomagają zarządzać wersjonowaniem bazy danych? Np. gdyby zadeklarowane przez ciebie create_schemas.sql się zmieniło, musiałbyś jakoś tą zmianę dodać, jak byś to zrobił? Co gwarantuje Ci, że pliki zostaną wykonane w prawidłowej kolejności?

Znam flyway i Liquibase, korzystałem trochę z Liquibase z tych dwóch. Nie wiem, czy jest tam jakaś opcja, żeby bezpośrednio zmienić plik, chyba nie. Można np zrobić changeset liquibase, dodać ten skrypcik jako referencję i wtedy już zmieniać rzeczy za pomocą liquibase. Bazowy skrypt pozostaje jako podstawa.

Aktualnie gwarancję, że pliki będą wykonane w prawidłowej kolejności daje to, że akurat ułożyły się alfabetycznie. Narzędziami do wersjonowania można sobie to zapewnić niezależnie od tego.

philemone · 2023-11-06T10:32:12Z

src/main/java/com/ramcel/cinema/reservation/functionalities/ticket/TicketType.java

+    ADULT(BigDecimal.valueOf(25)),
+    STUDENT(BigDecimal.valueOf(18)),
+    CHILD(BigDecimal.valueOf(12.5));


Fajnie, że użyłeś BigDecimal - a dlaczego nie np. Double? 🙂

Czy kwoty biletów moglibyśmy umieścić w innym miejscu, by nie musieć przebudowywać aplikacji gdy przyjdą np. podwyżki inflacyjne?

BigDecimal ma duży overhead, ale jest precyzyjne - za dużo obliczeń nie wykonujemy, więc chyba lepiej dokładnie policzyć ile chcemy dostać pieniędzy :).

Wartości biletów możnaby np wczytać z zewnętrznego pliku springa i konfigurować za pomocą Value, można zdefiniować Bean w konfiguracji springa. Są też pewnie jakieś metody na pobieranie tego z bazy danych, ale nie bawiłem się w to nigdy.

philemone · 2023-11-06T10:35:31Z

src/main/java/com/ramcel/cinema/reservation/Application.java

+import org.springframework.boot.autoconfigure.domain.EntityScan;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.annotation.ComponentScan;


Czy te importy są potrzebne?

philemone · 2023-11-06T10:36:15Z

target/classes/com/ramcel/cinema/reservation/Application.class

A co tutaj robi /target? 🙂

Wrzuciłem, bo bez niego mvn spring-boot czasem mi świrował (chyba miał jakiś problem z zewnętrznymi plikami)

philemone · 2023-11-06T10:36:42Z

.gitignore

-target/
-!.mvn/wrapper/maven-wrapper.jar
-!**/src/main/**/target/
-!**/src/test/**/target/


Celowo się pozbyłeś tych linijek?

philemone · 2023-11-06T10:38:14Z

src/test/java/com/ramcel/cinema/reservation/controller/SeatControllerTest.java

+import com.ramcel.cinema.reservation.functionalities.screening.Movie;
+import com.ramcel.cinema.reservation.functionalities.screening.Screening;
+import com.ramcel.cinema.reservation.functionalities.screening.ScreeningService;


philemone · 2023-11-06T10:55:13Z

...in/java/com/ramcel/cinema/reservation/functionalities/ticket/validators/TicketValidator.java

+        for (int i = 0; i < seats.size() - 2; i++) {
+            SeatEntity seat1 = seats.get(i);
+            SeatEntity seat2 = seats.get(i + 1);
+            SeatEntity seat3 = seats.get(i + 2);
+
+            if (seat1.getStatus() != SeatStatus.AVAILABLE
+                    && seat2.getStatus() == SeatStatus.AVAILABLE
+                    && seat3.getStatus() != SeatStatus.AVAILABLE) {
+                return false;
+            }
+        }
+        return true;


Czy można byłoby to zapisać bez użycia for? 🤔

Dałoby się to zapisać jakimś functional chainem, ale nie próbowałem być zbyt cwany - for był naturalnym rozwiązaniem dla takiego algorytmiku. Problemem zwłaszcza standardowych streamów w javie byłoby tutaj to, że zapominamy o poprzednim elemencie i ciężko byłoby w naturalny sposób porównać ze sobą 3 elementy. Da się, tylko chyba nie warto.

Cześć, pomyślałem jeszcze nad tym podpunktem chwile od wczoraj, bo stwierdziłem, że skoro pytasz, to jest jakiś sprytny myk jak to zrobić. Wymyśliłem coś takiego - do funkcji przekazujemy już listę jednego rzędu siedzeń, więc możemy posortować względem miejsca w rzędzie, odfiltrować puste miejsca i na koniec dać reduce, z warunkiem, że jeśli seatNr(i) + 2 = seatNr(i+1) , to rzucamy false - mamy dziurę. To działa, ale dla mnie jest chyba trochę zbyt skomplikowane w porównaniu do fora. Co o tym myślisz?
Jesli masz jakiś pomysł, jak to rozwiązać w iny sposób, to chętnie go poznam :)

Diamekod0221 · 2023-11-06T13:03:09Z

Dzięki za review, odpowiedziałem na wszystkie pytania.

Diamekod0221 added 25 commits October 18, 2023 16:59

Added basic service/controller outlines.

b4714fa

Added ScreeningServiceTesting

f8f862c

Added logging config, Screening tests.

07ac26b

Started writing TicketServiceImpl, started ticket validation.

1a809c5

Restructured project, wrote validation for single ticket. Started for…

e2260cb

… List.

Finished TicketValidator.java, added global exception handler.

dc9b90e

Finished core code. Time to start testing.

946dc28

Added tests for cvontrollers and Unit tests for services and subservi…

051a8c8

…ces.

Added tests for TicketService. Reordered logic to fix bugs in TicketV…

b7cd42f

…alidator

Fiished testing

bdcf05a

Fixed postMapping problem, improved exception handling and status codes.

a45ece7

changed exception handling. Fixed test cases.

444d9a9

Cleaned up tests.

e071b25

Added compiled to git

fbbbbe3

Added compiled to git

b34036a

Added sql scripts

9606f52

Added sql scripts

4c8a000

Added sql scripts

123330b

Added sql scripts

6b3c3ac

Added sql scripts

602d154

Added sql scripts

f2e6ded

Added sql scripts

9582832

Added sql scripts

4d33a8c

Added readme, how_to_run.txt, fixed script typos.

4d05730

philemone reviewed Nov 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full changes log merge from beginning of repo #1

Full changes log merge from beginning of repo #1

Diamekod0221 commented Nov 1, 2023

philemone left a comment

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023 •

edited

Loading

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023

philemone Nov 6, 2023

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023 •

edited

Loading

philemone Nov 6, 2023

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023

philemone Nov 6, 2023

philemone Nov 6, 2023

philemone Nov 6, 2023

Diamekod0221 Nov 6, 2023

Diamekod0221 Nov 7, 2023 •

edited

Loading

Diamekod0221 commented Nov 6, 2023 •

edited

Loading

		for SCRIPT in "$SQL_SCRIPTS_DIR"/*.sql; do
		SCRIPT_NAME=$(basename "$SCRIPT")

Full changes log merge from beginning of repo #1

Are you sure you want to change the base?

Full changes log merge from beginning of repo #1

Conversation

Diamekod0221 commented Nov 1, 2023

philemone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diamekod0221 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diamekod0221 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diamekod0221 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Diamekod0221 commented Nov 6, 2023 • edited Loading

Diamekod0221 Nov 6, 2023 •

edited

Loading

Diamekod0221 Nov 6, 2023 •

edited

Loading

Diamekod0221 Nov 7, 2023 •

edited

Loading

Diamekod0221 commented Nov 6, 2023 •

edited

Loading