Aplikacja do obstawiania meczy - prośba o code review

0

hej
Zacząłem pisać jakiś czas temu aplikację dla brata i mam jakąś pierwszą działającą wersję i prosiłbym o jakieś code review ;)

kod//github.com/krasnoludkolo/ebet2/

Ogolnie
System do obstawiania wygranych w meczach. Co jest:
-tworzenie lig i dodawnie do nich meczy
-dodawanie zakładów do meczy
-w momencie ustawienia wyniku meczu przelicza punkty na podstawie zakładów

Całość zrobiona w springu, ale użyłem go tylko tyle ile było niezbędne. Zamiast tworzyć wszystko jako beany, tylko moduły są beanami. Każdy moduł jest podzielony na część domenową (napisanej tylko w javie) zamkniętą za fasadą modułu oraz kod "infrastruktury" czyli kontrolery. W klasie konfiguracyjnej modułu podaję zewnętrzne zależności, takie jak np dostęp do bazy danych, co daję mi bardzo łatwe testowanie całej aplikacji w pamięci (dzięki temu brak mocków). Wydaję mi się, że taki podział w przyszłości pomoże np. w łatwy sposób wydzielić moduły jako mikroserwisy. Dodatkowo kod domenowy nie jest w żaden sposób zależny od IO, tylko czyta javav

Plany na przyszłość
-dodać baze danych inną niż w pamięci.
-pozbyć się springa
-zrobić jakieś podstawy cqrsa w postaci query
-osobny moduł do zarządzania użytkownikami
-readme na githubie

Jakie są wasze opinie i rady?

0

  1. Niemutowalność - jeżeli coś może być niemutowalne to powinno być, a jak nie może to powinno zostać.
    a.) efekt na JMM - final przy zmiennej to gwarancja *safe publication * ( Java Concurrency in Practice )
    b.) Pomoc dla Garbage collectora - krótko żyjące obiekty są super ! a niemutowalność temu sprzyja.

  2. Jakieś equals albo hashcode ?
    https://github.com/krasnoludkolo/ebet2/blob/master/src/main/java/pl/krasnoludkolo/ebet2/InMemorySystem.java

Option<BetDTO> betByUUID = betFacade.findBetByUUID(uuid);
        if (betByUUID.isEmpty()) {
            return new ResponseEntity<>(new BetDTO(), HttpStatus.NOT_FOUND);
        }
return new ResponseEntity<>(betByUUID.get(), HttpStatus.CREATED);

Optional ma metode ifPresent(Consumer<? super T> consumer) - wywal tego IFa plis XD

    @DeleteMapping("/bet")
    public HttpStatus removeBet(@RequestParam UUID uuid) {
        betFacade.removeBet(uuid);
        return HttpStatus.OK;
}

Zainteresuje się asynchronicznym przetwarzaniem, podobno sejm debatuje nad ustawą zabraniającą pisac kod w sposób blokujący (#takbyło) - np: tutaj https://spring.io/blog/2016/07/28/reactive-programming-with-spring-5-0-m1 (#teamSEDAtoSuabaOpcja)

  1. Ta struktura to jakaś masakra jest, tam jest więcej pakietów niż klas XD przemyśl to jeszcze raz.

  2. czujny :) new ConcurrentHashMap<>() - jest thread safe- myślałem że będzie jakaś arraylista :D

  3. Transakcja

    public UUID addBetToMatch(UUID matchUUID, NewBetDTO newBetDTO) {
        if (matchHasBetWithUsername(matchUUID, newBetDTO.getUsername())) {
            throw new DuplicateUsername();
        }
        String username = newBetDTO.getUsername();
        BetTyp betType = newBetDTO.getBetTyp();
        Bet bet = new Bet(matchUUID, username, betType);
        UUID uuid = bet.getUuid();
        repository.save(uuid, bet);
        return uuid;
}

Czy jesteś w stanie dostrzec problem związany z brakiem atomości ? (check-then-act)

[a1] -> [czy istnieje rekord w bazie?] -> [zapisz]
[a2] -> [czy istnieje rekord w bazie?] -> [zapisz]

  1. Ten Bet.class i mozliwość updatu BetTyp jest złe - niemutowalność.
0

@rubaszny_karp:
na szybko jeszcce wieczorem

Co do asynchroniczności. Tak jak wpisałem mam w planie dodanie jakiś podstaw cqrsa, i na tym poziomie zapewnienie tranzakcyjności.
1.Tak, miałem to przywrócić w DTOsach, ale jeszcze nie zdążyłem
2. Czemu akurat podałeś tę klasę jako brak equals albo hashcode?
3. Jak ifPresent tu pomoże, jeśli chce zwrócić wartość?
5. Przez dostęp pakietowy i dzięki temu przez hermetyzację wszystko siedzi na miejscu

1
danek napisał(a):

@rubaszny_karp:
na szybko jeszcce wieczorem

Co do asynchroniczności. Tak jak wpisałem mam w planie dodanie jakiś podstaw cqrsa, i na tym poziomie zapewnienie tranzakcyjności.
1.Tak, miałem to przywrócić w DTOsach, ale jeszcze nie zdążyłem
2. Czemu akurat podałeś tę klasę jako brak equals albo hashcode?
3. Jak ifPresent tu pomoże, jeśli chce zwrócić wartość?
5. Przez dostęp pakietowy i dzięki temu przez hermetyzację wszystko siedzi na miejscu

  1. jest hermetyczne i siedzi na miejscu ale tego się czytać nie da - pisze to jako osoba która widzi te strukturę pierwszy raz - może komu innemu się spodoba.
  2. bo nie ma equals i hashcode, generalnie jak masz DTOs, to zaimplementuj, to dwa kliknięcia w IDE
  3. chodzi mi o to że np: Optional zapewnia fajne API możesz zrobić
Option<BetDTO> betByUUID = betFacade.findBetByUUID(uuid);

return betByUUID.flatMap(szyszki ->  new ResponseEntity<>(betByUUID.get(), HttpStatus.CREATED))
                  .orElse(new ResponseEntity<>(new BetDTO(), HttpStatus.NOT_FOUND));

zamiast

Option<BetDTO> betByUUID = betFacade.findBetByUUID(uuid);
    if (betByUUID.isEmpty()) {
        return new ResponseEntity<>(new BetDTO(), HttpStatus.NOT_FOUND);
    }
return new ResponseEntity<>(betByUUID.get(), HttpStatus.CREATED);
1
  1. czy wyjątki nie są częścią api?
  2. BetCRUDService - kiepska nazwa, dodasz coś bardziej skomplikowanego i przestanie być CRUD. Drugi człon nazwy również nic nie mówi - serwis od akcji typu CRUD na betach. W gruncie rzeczy to repository, które dokonuje odpakowania DTOsów. Te filtrowania to wg mnie powinno robić repozytorium, bo wywołanie findAll, a potem filtrowanie w pamięci mogłoby się źle skończyć jak rekordów przybędzie. Tym sposobem BetCRUDService nie ma żadnej odpowiedzialności
  3. repository.findAll().filter(b -> b.getMatchUuid().equals(matchUUID)).map(Bet::getUsername).contains(username); - kiepska czytelność takiego 1 linijkowego potwora
  4. masz pakiety league i result, a w result np. LeagueResult - daje do myslenia czy to na pewno powinno byc osobno
  5. w testach też mozesz lepiej asercje napisac, gdzie masz optiony, ja np. waliłbym od razu get jeżeli test zakłada, że taki obiekt tam ma być, jak nie będzie to i tak wybuchnie

also @rubaszny_karp

Ten Bet.class i mozliwość updatu BetTyp jest złe - niemutowalność.

co w tym złego?

co do projektu - za mało by oceniać, mało się tam dzieje 'mięska'. Jest clean architecture, chcesz to dalej rozwijać (cqrs np), ale ta aplikacja dość mało robi, więc ominą cię problemy, które będziesz musiał rozwiązać na większych systemach, a na pierwszy rzut oka może się faktycznie wydawać taka architektura przerostem formy nad trescia

Generalnie jest OK

0

@hcubyc:

hcubyc napisał(a):
  1. czy wyjątki nie są częścią api?

hm, mogą faktycznie być

  1. BetCRUDService - kiepska nazwa, dodasz coś bardziej skomplikowanego i przestanie być CRUD. Drugi człon nazwy również nic nie mówi - serwis od akcji typu CRUD na betach. W gruncie rzeczy to repository, które dokonuje odpakowania DTOsów. Te filtrowania to wg mnie powinno robić repozytorium, bo wywołanie findAll, a potem filtrowanie w pamięci mogłoby się źle skończyć jak rekordów przybędzie. Tym sposobem BetCRUDService nie ma żadnej odpowiedzialności

Chciałem sprawić aby fasada tylko wywoływała inne obiekty, które już by dalej wykonywały jakieś operacje, np tworząc obiekt walidowały, sprawdzały jakieś warunki itp. Faktycznie obecna nazwa nie jest zbytnio fortunna, ani czytelna. Jakąś proponujesz?

  1. repository.findAll().filter(b -> b.getMatchUuid().equals(matchUUID)).map(Bet::getUsername).contains(username); - kiepska czytelność takiego 1 linijkowego potwora

Chodzi tylko o formatowanie czy coś więcej?

  1. masz pakiety league i result, a w result np. LeagueResult - daje do myslenia czy to na pewno powinno byc osobno

Może znowu nazewnictwo niezbyt fortunne. Moduł League jest od samego zarządzania, tworzeniem itp meczy i lig, a moduł result od liczenia punktów.

  1. w testach też mozesz lepiej asercje napisac, gdzie masz optiony, ja np. waliłbym od razu get jeżeli test zakłada, że taki obiekt tam ma być, jak nie będzie to i tak wybuchnie

Jest to jakiś pomysł

co do projektu - za mało by oceniać, mało się tam dzieje 'mięska'. Jest clean architecture, chcesz to dalej rozwijać (cqrs np), ale ta aplikacja dość mało robi, więc ominą cię problemy, które będziesz musiał rozwiązać na większych systemach, a na pierwszy rzut oka może się faktycznie wydawać taka architektura przerostem formy nad trescia

Generalnie jest OK

Stwierdziłem, że lepiej jest mi teraz "zainwestować" i zastanowić się jak to dalej ciągnąć, zwłaszcza, że mam "klienta" (mój brat, dla którego to robię) więc pomysły na dalszy rozwój pewnie będą 

Dzięki za uwagi ;)

1

also @rubaszny_karp

Ten Bet.class i mozliwość updatu BetTyp jest złe - niemutowalność.

co w tym złego?

Do repozytorium ktory jest concurrentHashMapą wrzucam obiekt potem go modyfikuje - wątki na moim sockecie widzą zmiany (bardzo prawdopodobne, ten sam cache) inne go nie widzą - syf.

0

@rubaszny_karp:
Fakt, zwracam honor. Chociaż za jakiś czas pewnie bedize baza danych lub inny storage i IMHO spoko jest miec mutowalne obiekty biznesowe, jezeli technologia na to pozwala i nie wyciekaja nigdzie, ale to nie ten przypadek

Chodzi tylko o formatowanie czy coś więcej?

Chodzi o formatowanie. Operacja na linie jest IMHO rozsądnym podejściem (w 1 linii robię streama, w drugiej filtruje, w trzeciej zwracam listę)

Chciałem sprawić aby fasada tylko wywoływała inne obiekty, które już by dalej wykonywały jakieś operacje, np tworząc obiekt walidowały, sprawdzały jakieś warunki itp. Faktycznie obecna nazwa nie jest zbytnio fortunna, ani czytelna. Jakąś proponujesz?

To dobre podejście moim zdaniem, tylko masz generyczne repozytorium i stąd ten serwis u ciebie. W obecnym kształcie to wszystko mogłaby fasada robić, bo nie ma tam logiki biznesowej i ona mogłaby wołać repo. Przepakowywanie dtosa w obiekt domenowy to mógłby robić obiekt domenowy (np. static factory method fromDto) albo jakaś fabryka, chociaż tu fabryka IMHO też byłaby przerostem formy nad treścią. To moje zdanie, możesz mieć twojsze, ale ciężko zaproponowac nazwę obiektowi, który w sumie nie wiadomo co robi ;)

A co do equals i hashcode to jeżeli generujesz przez IDE to warto rozważyć skorzystanie z lomboka, skoro i tak już korzystasz ;)

0
hcubyc napisał(a):

Chodzi tylko o formatowanie czy coś więcej?

Chodzi o formatowanie. Operacja na linie jest IMHO rozsądnym podejściem (w 1 linii robię streama, w drugiej filtruje, w trzeciej zwracam listę)

A co powiesz o takim pisaniu "prozą"?


    void addPointToUser(String username) {

        UserResult result = userResultList
                .find(withName(username))
                .getOrElse(createNewUserResult(username));


        result.addPoint();
        userResultList = userResultList.append(result);
    }

    private Supplier<UserResult> createNewUserResult(String user) {
        return () -> new UserResult(user);
    }

    Option<UserResult> getUserResultForName(String username){
        return userResultList.find(withName(username));
    }

    private Predicate<UserResult> withName(String user) {
        return userResult -> userResult.hasName(user);
    }

A co do equals i hashcode to jeżeli generujesz przez IDE to warto rozważyć skorzystanie z lomboka, skoro i tak już korzystasz ;)

Lombok z tym momentami ma problemy ;)

Generalnie nie ma wstydu i do cv można by podpiąć?

0

Wstydu nie ma, można podpiąc do CV, jak będzie bardziej rozbudowana apka tym bardziej.
Jeżeli chodzi o to:

userResultList
                .find(withName(username))

to dlaczego nie:

userResultList
                .findByName(username)

Czy ja wiem, czy lombok ma problemy. Ktoś pisał na forum, że trzaśnie stackoverflow generujac equals i hashcode jeżeli masz cykliczną zależność miedzy klasami, ale cykl jest większym problemem niż ten SO

0
hcubyc napisał(a):

Wstydu nie ma, można podpiąc do CV, jak będzie bardziej rozbudowana apka tym bardziej.

Cały czas teraz rozwijam więc zapewne zanim ktoś przeczyta to już coś nowego będzie ;)

to dlaczego nie:

userResultList
                .findByName(username)

userResultList to lista z vavra i ma metodę find.

1 użytkowników online, w tym zalogowanych: 0, gości: 1