Ocena projektu do portfolio

Witam, zaczynam tworzyć CV i właśnie kończę projekt który chciałbym tam dodać. Czy możecie mi powiedzieć czy się nada i ewentualnie co trzeba poprawić? Nie chce rozsyłać czegoś na odwal więc liczę na waszą pomoc.
https://github.com/LukaszCh233/quiz_World

Niezły, jak na polskie standardy powinno być okej.

Ale oczywiście testy są 2/10; nie chodzi tutaj o Ciebie, bo takie testy są w większości firm, ale nadal są 2/10.

Np to:

@Test
void shouldCreateUser_ExistingEmail() {
    //Given
    User user = new User(null, "user", "email", "password", Role.USER);
    ///when

    userRepository.save(user);
    //then
    assertThrows(ExistsException.class, () -> userService.createUser(user));
}

powiedziałbyś że to dobry test?

Wszystkie te: WordSetCategoryServiceTest, WordSetServiceTest, WordsServiceTest - to są testy które będą Ci przeszkadzać edytować Twój projekt. Jak tylko przyjdzie jakaś zmiana w tym rejonie, to przez te testy ta zmiana będzie trudna. W tym momencie większość ludzi dochodzi do wniosku że "pisanie testów spowalnia". Tymczasem to nie pisanie testów spowalnia, tylko pisanie słabych testów spowalnia.

Czytając testy powinieneś móc stwierdzić co kod robi. Czytam Twoje testy i nie mam pojęcia co kod robi - ergo testy nie są czytelne.

Praktycznie wszystkie Twoje testy w tej aplikacji nie testują zachowania, tylko przyklepują implementację. Jak mówiłem - to jest standard w większości firm, ale nie mniej to są bardzo słabe testy. Zaczną Ci przeszkadzać bardzo szybko.

@Riddle: No z testów nie jestem zadowolony ale zrobiłem je żeby były. Domyślam się, że nie wiele one testują ale no jak uczą tak robię. Jak bym wiedział jak zrobić to lepiej to bym zrobił. Najgorsze to chyba testy controllerow, to całkiem bez sensu się wydaje co napisałem.

LukaszCh233 napisał(a):

@Riddle: No z testów nie jestem zadowolony ale zrobiłem je żeby były. Domyślam się, że nie wiele one testują ale no jak uczą tak robię.

Kwintesencja pracy w polsce. "Wiem że bez sensu, ale zrobię". No cóż, nie Twoja wina, firmy tak pracują. Chociaż tyle że wiesz, że są do bani.

LukaszCh233 napisał(a):

Jak bym wiedział jak zrobić to lepiej to bym zrobił. Najgorsze to chyba testy controllerow, to całkiem bez sensu się wydaje co napisałem.

No tak.

Riddle napisał(a):

LukaszCh233 napisał(a):

@Riddle: No z testów nie jestem zadowolony ale zrobiłem je żeby były. Domyślam się, że nie wiele one testują ale no jak uczą tak robię.

Kwintesencja pracy w polsce. "Wiem że bez sensu, ale zrobię". No cóż, nie Twoja wina, firmy tak pracują. Chociaż tyle że wiesz, że są do bani.

LukaszCh233 napisał(a):

Jak bym wiedział jak zrobić to lepiej to bym zrobił. Najgorsze to chyba testy controllerow, to całkiem bez sensu się wydaje co napisałem.

No tak.

Od czegoś trzeba zacząć :) może się nauczę w pracy jak sie uda gdzieś dostać. Pewnie nie przeglądałeś jakoś szczegółowo ale może jakieś uwagi masz? C
hce żeby było elegancko dopracowane.

Dobrym punktem wyjścia do napisania testów do Twojej aplikacji byłoby Twoje readme. Dlaczego? Dlatego że tylko z tego ReadMe udało mi się domyślić co Twoja aplikacja robi Nie z kodu (bo nic nie widać z niego), nie z testów (bo one testują tylko implementację), tylko z ReadMe; jedyne miejsce gdzie jest CHOCIAŻ trochę info co Twoja aplikacja robi.

Pozwoliłem sobie skopiować Twoje readme z repozytorium:

Quiz World it's My project in Java and Spring Boot where We have two moduls. Application provide two options for Admin and for User. Admin who can manage the application and User who uses the application.

Quiz - modul with quizzes where We have many functions like, create, modifications, delete, solve quizzes and check your score and other people.

Words - modul where We have word sets and We can learn new words. Many functions like create private word sets or for everyone, modifications, delete, solve word sets.

To dużo bardziej brzmi jak dobre testy, niż cokolwiek co napisałeś.

Moja rada:

1. Wywal dosłownie wszystkie testy jakie masz w folderze test/ - dlatego że one nic nie testują, nie specyfikują założeń. Testy które napisałeś przyklepują implementację, a to jest jeszcze gorsze niż by ich w ogóle nie było.
2. Weź Twoje ReadMe, i zauważ w nim test-case'y. Ja widzę takie:

• Quiz World it's My project in Java and Spring Boot where We have two moduls.
  

  Nieistotne, szczegół implementacyjny.

• Application provide two options for Admin and for User. Admin who can manage the application and User who uses the application.
  

  Już masz dwa testy:

  • Administrator może zarządzać aplikacją: @Test void adminManagesApplication()
  • Użytkownik może użyć aplikacji: @Test void userUsesApplication()

  Widocznie te dwa elementy są ważne, a skoro tak, to powinny mieć testy.

• Quiz - modul with quizzes where We have many functions like, create, modifications, delete, solve quizzes and check your score and other people.
  

  Testy się sypią z rękawa: create, modifications, delete, solve quizes, check your score and other people:

  • @Test void createQuiz(), @Test void quizModifications(), @Test void deleteQuiz(), @Test void checkUserScore(), @test void checkOtherPeople()

• Words - modul where We have word sets and We can learn new words. Many functions like create private word sets or for everyone, modifications, delete, solve word sets.
  

  Kolejne testy się aż proszą:

  • We have word sets -> @Test void wordSets()
  • We can learn new words -> należałoby napisać jakiś test pod to, to zależy od tego w jaki sposób się "learn tych new words"
  • create private word sets -> @Test void createPrivateWordSet()
  • or for everyone -> @Test void createPublicWordSet()

Po prostu wziąłęm wszystkie funkcje które ma mieć Twój program, i przerobiłem je w przypadki testowe. W taki sposób należałoby do tego podejść. Pisz testy pod to, co program ma robić - tworzyć quizy, dodawać słowa, sprawdzać score (nie to w jaki sposób to robi, endpoint, service, repository, to są szczegóły implementacyjne).

@Riddle: README jeszcze jest do dopracowania bo nie skończyłem więc postaram się zastosować to co piszesz. Czyli dobrze rozumiem, że nie pisać testów tylko po prostu napisać to w README?

LukaszCh233 napisał(a):

@Riddle: README jeszcze jest do dopracowania bo nie skończyłem więc postaram się zastosować to co piszesz. Czyli dobrze rozumiem, że nie pisać testów tylko po prostu napisać to w README?

Nie o to mi chodziło.

Miałem na myśli to, że testy powinny testować "zachowanie". Czyli co aplikacja robi: pozwala dodawać quizy, edytować quizy, liczyć score. Takie testy powinny się pojawić. Nie powinieneś pisać testów, które wiedzą tak dużo o kontrollerach, modelach, route'ach, etc. - bo to za bardzo przywiązuje do implementacji.

Testy mają Ci pomóc refaktorować i zmieniać system, pozwolić na zmianę kodu; jednocześnie sprawdzając czy program nadal ma robić to co robił.

W moim poście z ReadMe chodziło o to, że z ReadMe dowiedziałem się co Twój program tak na prawdę robi - bo ReadMe to było jedyne miejsce w Twoim repozytorium, w którym na prawdę była jakakolwiek informacja o zachowaniu Twojego systemu, co on na prawdę miał robić. Dlatego potraktowałem to jako punkt wyjścia.

Jeśli chcesz napisać testy dobrze, to zrób tak:

1. Zastanów się co Twój program powinien robić, z punktu widzenia użytkownika, wysokopoziomowo. Takim zachowaniem mogłoby być np. "stwórz zestaw słów" albo "podziel się zestawem z innymi użytkownikami" albo "odczytaj wynik quizu".
2. Zastanów się jak takie zachowanie najprościej sprawdzić testem
3. Napisz taki test

I tyle.

@Test
void shouldCreateUser_ExistingEmail() {
    //Given
    User user = new User(null, "user", "email", "password", Role.USER);
    ///when

    userRepository.save(user);
    //then
    assertThrows(ExistsException.class, () -> userService.createUser(user));
}

Ja naniósłbym abstrakcję na szczegóły implementacyjne w Twoich testach.

  @Test
    void whenCreateExistingUser_ThrowException() {
        createExistingUserByUsername("Alex");
        assertThrows(ExistsException.class, () -> createUser("Alex"));
    }

Teraz, gdy wprowadzisz zmiany w kodzie, nie będziesz musiał modyfikować testów, a jedynie abstrakcję.

Edit.
Może warto byłoby też ukryć wyjątek i zrobić tak:

assertThrowsException(createUser("Alex"));

Jest nieźle, serio. Coś takiego już jest jakąś podstawą do rozmowy.

Parę rzeczy które od razu widzę i są łatwe do poprawy:

• nazwa pakietu powinna być zgodna z javową konwencją, czyli małe litery i bez "_"
• skoro zdecydowałeś się używać Lomboka, to CustomUserDetailsService możesz opędzlować AllArgsConstructor - nie ma potrzeby ręcznego wbijania zależności
• użyj Dockera do zależności - nie pisz "potrzebujesz pgAdmina i Postgresa w wersjach x, y" bo nikomu nie będzie się chciało nawet tego uruchamiać, tylko po prostu dostarcz docker-compose.yml który od razu te zależności uruchomi
• tak naprawdę nie potrzebujesz Mavena - przecież masz wrapper w repo :)
• konfiguracja Postgresa w Dockerze kompletnie skasuje cały krok "Configure Database" - nie będziesz musiał tego tłumaczyć
• wiem że to demko ale HelpJwt.getSignInKey chociaż daj ten "losowy" string do propsów, bo wygląda to śmiesznie
• JwtTokenServiceImpl nic nie implementuje. IntellIJ zaznacza na szaro - to chyba nie jest używane?
• pomyśl o "package by feature"
• MapEntity jest beanem, ale wcale nie musi być, to po prostu worek na statyczne metody.
• AdminService jest identyczny z UserService tylko minimalnie nazwy inne. Fajnie to widać jak się przełącza pliki w IntelliJ. Na pewno nie da się tego połączyć?

@kelog: Ty to potrafisz podnieść na duchu xD z dockerem nie miałem jeszcze styczności bede musiał poczytać i postaram sie to poprawić.
Ogólnie to starłem się podzielić to jakoś żeby nie było za dużo metod w jednej klasie. Uznałem że lepiej rozdzielić user i admin.

Riddle napisał(a):

Jeśli chcesz napisać testy dobrze, to zrób tak:

1. Zastanów się co Twój program powinien robić, z punktu widzenia użytkownika, wysokopoziomowo. Takim zachowaniem mogłoby być np. "stwórz zestaw słów" albo "podziel się zestawem z innymi użytkownikami" albo "odczytaj wynik quizu".
2. Zastanów się jak takie zachowanie najprościej sprawdzić testem
3. Napisz taki test

I tyle.

Czyli jak mam np. wyświetlanie spisu quizów to mam po prostu napisać test używając metody do wyświetlania listy quzizów?

.

LukaszCh233 napisał(a):

@kelog: z dockerem nie miałem jeszcze styczności bede musiał poczytać i postaram sie to poprawić.

Obecnie to chyba must have.

To co ja bym ogarnął:
Docker nie jest trudny do podstawowego setupu, a daje dużo.

W pracy się nauczysz poprawnie tworzyć nazwy (czyli nie helpJwt, tylko JwtHelper, nie używasz w nazwie klasy czasownika), ale tutaj nie wiem do końca czemu stworzyłeś dwa osobne byty: Admin i User, to są dwie strony tej samej monety, a różniące się jedynie rolą (to rola powinna decydować kto ma jakie uprawnienia).

Logowanie systemu do debugu: widzę, że logujesz flow, nie akcję. Dla programisty ważne jest co i gdzie poszło nie tak. Spójrzmy na controller - mamy tam info czy logowanie się udało, czy nie - a to powinno być na poziomie funkcji (raczej prywatnych funkcji się nie loguje). Logowanie powinno wyglądać jak stack trace.
Zobacz na dokumentację spring security, możesz sobie sparametryzować error przy 401.

Czyli masz endpoint registerAdmin, to fajnie żebyś wiedział, że coś uderzyło na ten endpoint, np. "Received register admin request: {}, admin", masz serwis i tam fajnie jakby na początku było info "Creating admin: {}, admin" i potem po tej informacji jakby coś poszło nie tak to masz pyk, exception, normalnie spodziewana sciezka success nie jest logowana - po to jest zwracane Http.OK.

Poprawisz te rzeczy, zrobisz package by feature usuwając przy okazji w większości klas i funkcji publiczne modyfikatory dostępu i będzie super projekt :)

Ogólnie pole do poprawy jest ogromne, ale szczerze to już jest spoko jak na uderzanie na juniora :)

Erip222 napisał(a):

To co ja bym ogarnął:
Docker nie jest trudny do podstawowego setupu, a daje dużo.

Właśnie się będę za to zabierał coś poczytałem i faktycznie bardzo pomocny sie wydaje.

W pracy się nauczysz poprawnie tworzyć nazwy (czyli nie helpJwt, tylko JwtHelper, nie używasz w nazwie klasy czasownika), ale tutaj nie wiem do końca czemu stworzyłeś dwa osobne byty: Admin i User, to są dwie strony tej samej monety, a różniące się jedynie rolą (to rola powinna decydować kto ma jakie uprawnienia).

Czyli po prostu zostawić User i service tez zrobić jeden w sumie logiczne to sie wydaje.

Logowanie systemu do debugu: widzę, że logujesz flow, nie akcję. Dla programisty ważne jest co i gdzie poszło nie tak. Spójrzmy na controller - mamy tam info czy logowanie się udało, czy nie - a to powinno być na poziomie funkcji (raczej prywatnych funkcji się nie loguje). Logowanie powinno wyglądać jak stack trace.
Zobacz na dokumentację spring security, możesz sobie sparametryzować error przy 401.

Logowania sie pozbyłem w innych klasach tylko zostało w access i nie wiem czy to zostawiać czy nie.

Czyli masz endpoint registerAdmin, to fajnie żebyś wiedział, że coś uderzyło na ten endpoint, np. "Received register admin request: {}, admin", masz serwis i tam fajnie jakby na początku było info "Creating admin: {}, admin" i potem po tej informacji jakby coś poszło nie tak to masz pyk, exception, normalnie spodziewana sciezka success nie jest logowana - po to jest zwracane Http.OK.

Poprawisz te rzeczy, zrobisz package by feature usuwając przy okazji w większości klas i funkcji publiczne modyfikatory dostępu i będzie super projekt :)

Ogólnie pole do poprawy jest ogromne, ale szczerze to już jest spoko jak na uderzanie na juniora :)

Mam problem package by featur bo staram sie to jakoś dobrze podzielić ale ciągle nie wychodzi z tego co widze po komentarzach. Podzieliłem wszystko na wiecej klas żeby nie było wszystko w jednej, porobiłem katalogi.
Dzięki za rady i dobre słowo, człowiekowi aż się chce działać jak zobaczy coś miłego :)

Moje to chyba jest package by layer czy nie bardzo? I czemu akurat by feature jest lepszym rozwiązaniem?

LukaszCh233 napisał(a):

Moje to chyba jest package by layer czy nie bardzo? I czemu akurat by feature jest lepszym rozwiązaniem?

Żadne nie jest lepszym rozwiązaniem. Po prostu masz wyznawców różnych form pakietowania. Tak jak w bieganiu masz wyznawców różnych rodzajów obuwia.

Np. Moim zdaniem najlepszym pakietowaniem jest skorzystanie z architektury Portów i Adapterów (tzw. Onion Architecture), potem szedłbym w "package by feature". Architektura warstwowa też jest bardzo dobra i ma swoje zastosowania. U mnie jest na 3 miejscu.

Żadne nie jest lepszym rozwiązaniem

Może subiektywne, ale nie zgadza się to z moim doświadczeniem. Pracowałem w typowym monolicie w architekturze warstwowej i fakt - może te warstwy były źle zrobione - nie wiem, ale widząc 200 klas publicznych w pakiecie services, 100 klas publicznych w pakiecie controllers i reszta podobnie - nie miało to wg mnie żadnego sensu.

Później dodając nowe ficzery zacząłem robić bardziej package-by-feature i było to znacznie znośniejsze.

To już nie wiem jak mam w końcu to podzielić dobrze xD na dodatek męczę się z tym dockerem

np. lepiej jak zrobie Quiz, Words, User i w Quiz dam wszystko co związane z quizem. Zamiast AdminController, CommonController i UserController będzie to robie katalog quizController i tam tworze UserQuizController CommonQuizControler i AdminQuizController. Tak samo robie z Words?

Druga sprawa podeśle ktoś jak jakiś dobry poradnik do dockera bo GPT nie współpracuje chyba no i też nie moge znaleźć rozwiązania w necie bo mam problem z "/target/quiz-world-0.0.1-SNAPSHOT.jar": not found.

Wygląda to tak nie wiem w czym jest problem jeszcze testy mi sie wywaliły przez valid i nie wiem czy je poprawić czy wywalić i napisać nowe

.jar.original na .jar i powinno być OK

Productionserver napisał(a):

.jar.original na .jar i powinno być OK

Że jak? Przecież tak mam właśnie

Skąd wziąłeś WORKDIR /app? Bo jeśli z chatgpt, to polecam zrozumieć co wypluwa ;) Nie masz nawet takiego katalogu.

Pokaż jeszcze jak budujesz, bo nie widać komendy. IMO docker build -t <tag> . w katalogu głównym projektu (tam gdzie jest target) i wywalić WORKDIR.

EDIT: czekaj, chyba nie rozumiem jak to działa jednak :P to chyba coś innego robi. Nie zmienia to faktu, że jest niepotrzebne. Możesz w kontenerze spokojnie mieć app.jar w katalogu głównym / i wtedy nie ma problemu.

Czyli: FROM ... jest ok, COPY ... /app.jar i CMD ... /app.jar - dawaj wszędzie ścieżki absolutne, po co się głowić gdzie co jest :)

kelog napisał(a):

Skąd wziąłeś WORKDIR /app? Bo jeśli z chatgpt, to polecam zrozumieć co wypluwa ;) Nie masz nawet takiego katalogu.

Pokaż jeszcze jak budujesz, bo nie widać komendy. IMO docker build -t <tag> . w katalogu głównym projektu (tam gdzie jest target) i wywalić WORKDIR.

EDIT: czekaj, chyba nie rozumiem jak to działa jednak :P to chyba coś innego robi. Nie zmienia to faktu, że jest niepotrzebne. Możesz w kontenerze spokojnie mieć app.jar w katalogu głównym / i wtedy nie ma problemu.

Czyli: FROM ... jest ok, COPY ... /app.jar i CMD ... /app.jar - dawaj wszędzie ścieżki absolutne, po co się głowić gdzie co jest :)

Dobra zadziałało, usunąłem wszystko z dockerignore i poszło.
Tylko jak moge to teraz sprawdzić czy działa bo chyba nie do końca rozumiem co to ma robić.
Teraz po prostu jak ktos pobierze projekt z githuba i odpali to nie bedzie musiał pobierać bazy danych i zależności i konfigurować tego?

Na razie zbudowałeś obraz ze swoją apką. Z tego co Ci sugerowali to wypchnięcie infrastruktry do docker-compose, to są 2 różne rzeczy. Idea jest taka, pobieram repo, odpalam "docker compose up", docker stawia mi bazę danych i inną potrzebną infrę. Dlatego potrzebujesz docker-compose.yml. Możesz też dodać stworzony właśnie obraz swojej aplikacji jak wypchniesz na DockerHuba, ale to już krok dalej

@Productionserver:
Rozumiem, czyli to trochę więcej zabawy z tym będzie. Bardziej to zrozumiem jak będę mógł sam użyć tego na czyimś projekcie i docker skonfiguruje za mnie wszystko co potrzebne bo tak to rozumiem.

docker skonfiguruje za mnie wszystko co potrzebne bo tak to rozumiem.

Tak i nie. Wyobraz sobie, ze jest jakis projekt, ktory - z jakiegos powodu - wymaga polaczenia ze wszystkim z nastepnujacych: PostgreSQL, Redis, Keycloak, Kafka. Malo tego! Musi sie tez zgadzac tzw. major wersja serwera, wiec musi byc np. Keycloak 18 a nie 24. I tak dla kazdej zaleznosci. No przeciez kurna nie bedziesz kazdej uslugi stawial reczniej w swoim lokalnym systemie. W dodatku przelacz sie na inny projekt - albo nawet brancha - ktory wymaga innej wersji dla jednej z tych uslug.... no masakra. Po to wlasnie jest Docker. Robisz docker compose up i masz wszystko ogarniete.

I dockerowy obraz twojej docelowej aplikacji to na tym etapie nie jest w ogole potrzebny. Chodzi o zaleznosci bardziej.

I sam Docker jest raczej prosty w obsludze. Bo takie przykladowe uruchomienie uslugi Postgresa to wyglada w ten sposob: docker run --rm -it -d --name dockerpostgres -p 5432:5432 -e POSTGRES_USER=dev -e POSTGRES_PASSWORD=dev postgres:15 - tylko tyle, na prawde.
Tylko zazwyczaj stawiasz kilka uslug i stad w tej calej ukladance pojawia sie docker compose

@stivens: No w sumie mi tylko trzeba dockera żeby nie musieli tego konfigurować tylko pobrać i żeby było wszystko co potrzeba. Raczej chyba nikt nie odpala projektu przy rekrutacji i sprawdza wszystko bo ich chyba interesuje sam kod. Muszę poczytać więcej i się tym pobawić bo niby coś tam świta ale nie do końca.

Jesli ktos tutaj zwrocil Ci uwage, ze nie ma Dockera, to czemu myslisz, ze ktos patrzac na repo, pod katem rekrutacyjnym, to nie zwroci na to uwagi?

Pomijajac juz to, ze to nie chodzi o rekrutacje, a o to zebys sie orientowal w praktykach