Code review.

0

Na wstępie chciałbym powiedzieć, że będę wdzięczny za jakikolwiek odzew.
Planuję porzucić mój projekt który pisałem "jakiś czas", jednocześnie ucząc się Springa oraz testowania. Problem pojawił się w chwili, gdy zaczęło dochodzić do refactoringu oraz pisania testów, ponieważ nie skończyłem ani jednego z modułów, a miałem już ogólny zarys aplikacji (począwszy od logowania, przechodząc przez jakąś logikę czy jakieś walidacje pól). Zmienne, metody nazywałem niepoprawnie, ze strony gita - wszystko szło na jednym branchu i wiele więcej niepoprawnych rozwiązań. Toteż postanowiłem, poprosić Was o code-review (wytykanie błędów mile widziane), a jednocześnie mam kilka pytań:
1.Zmienne Sesyjne - jeżeli w aplikacji planujemy mieć system logowania (tak jak u mnie), to czy sposób z utworzeniem komponentu np SessionVariables o zasięgu @SessionScope i w każdym miejscu (panel użytkownika - UserService, dodawanie ofert kupna danej rzeczy np kryptowaluty jak na giełdzie https://www.binance.com/en/trade/ETH_BTC - CryptoCurrencyService) utworzyć obiekt tej klasy jako pole (wcześniej wspomnianego UserService czy CryptoCurrencyService) jest poprawny? Potem w sytuacji, gdybyśmy chcieli dodać ofertę wystarczyłoby:

sessionVariables.getLoggedUser().addOffer(offer);

Minusem czegoś takiego jest, mieszanie domeny Użytkownika z Kryptowalutą.
2. Mamy dwa serwisy: FooService i BooService. Co w sytuacji, gdy Foo i Boo są zupełnie innymi (tzn Foo i Boo są osobnymi agregatami [o ile dobrze zrozumiałem czym są agregaty]) klasami @Entity, więc w FooService powinno być np. FooRepository a w BooService - BooRepository. Chciałbym wykorzystać obiekt klasy Boo pobierając go z bazy danych w FooService? Czy utworzenie BooRepository w FooService jest błędem (ze względu na czystość kodu)? Jeżeli nie, to jak uzyskać komunikację serwisów? Jak sprawić, żeby FooService "poprosiło" BooService, żeby ten "poprosił" BooRepository żeby pobrał coś z bazy danych?
3. Jak testować, czy dwie zmienne double są sobie równe? Assert.assertEquals(double,double,epsilon) jest chyba przestarzałe, ale czy przy założeniu, że podczas testu np, pomimo tego, że te dwie zmienne są double są tak naprawdę całkowite (+- epsilon maszynowy) i wzięciu epsilon na tyle dużego, żeby wyeliminować błąd ale na tyle małego, żeby nie zakłamać wyniku (np 0.1) można używać assertEquals, czy jest jakaś alternatywa do tego?
4. Korzystam z thymeleafu, nie znam się na js a thymyleaf w zupełności mi wystarcza żeby sprawdzić ręcznie czy coś działa, jaki powinien być zwracany typ w kontrolerach? O ile wiem, że można zwrócić zwykłego Stringa, który przedstawia nazwę template'ki o tyle nie podoba mi się, że za każdym razem zwracam zwykłego Stringa.
5. Kontroler nie powinien mieć żadnej logiki. Czy takie coś w kontrolerze jest dopuszczalne, ze względu na czysty kod:

if(someCondition) return "u1";
else return "u2";
  1. Walidacja za pomocą setterów. Czy jest poprawna? Np w sytuacji gdy byśmy chcieli dopuścić, żeby imię użytkownika miało określony wzorzec. Jeżeli nie będzie - rzucenie wyjątkiem. Plus jest taki, że pozostali programiści nie muszą wiedzieć, czy jest jakaś dodatkowa klasa która pełni rolę walidatora (czy pośrednika), mamy walidację gdy w bazie danych będą złe dane (używając jakiegoś Hibernate czy SpringData). Minusem jest to, że rzucamy wyjątek (słyszałem, że rzucanie wyjątkiem w sytuacji gdy dana sytuacja może się bardzo często powtarzać nie jest czymś dobrym). Kolejnym ciekawym rozwiązaniem wydaje się zrobienie konstruktora klasy @Entity protected i utworzeniem jakiejś np. fabryki w tym samym pakiecie która by się komunikowała z resztą aplikacji. Plus zmiana:
Foo foo = new Foo(); //w konstruktorze jakieś zmienne
// na
Optional<Foo> foo = fooFactory.createFoo(); //w konstruktorze jakieś zmienne

O tyle fajne z Optionalem jest to, że nie rzucamy wyjątkiem.
7. Czy używając embeded database w testach, jak np H2, trzeba tworzyć plik konfiguracyjny? Czy wystarczy w pliku pom (jeżeli korzystam z mavena) główną bazę danych dać scope = runtime, a H2 scope = test? Sprawdzałem, wywalając konfigurację głównej bazy danych podczas testowania i tak - testowy kontekst aplikacji wstawał gdy główny nie mógł, więc chyba wystarczy.
8. Wiem, że powinienem usunąć konfigurację DB z application.properties i przerzucić to do zmiennych środowiskowych.
9. https://github.com/KamLar/market-simulator <-- Tak jak mówiłem, jeśli ktoś ma czas i mógłby spojrzeć i oprócz tego co napisałem wytknąć jeszcze jakieś błędy, ciekawe rozwiązania będę wdzięczny.

1
  1. Trochę zależy od tego co to faktycznie robi. Na suchych przykładach z foo i bar trochę trudno mi na to odpowiedzieć ;)
  2. Zdecydowanie nie używać doubli w ogóle. Lepiej jakieś BigIntiger do obliczeń czy coś takiego
  3. Zazwyczaj (no, podobno...) część backendowa zwraca dane w postaci jsona które są potem obrabiane przez front, czyli w springu możesz zwrócić w sumie cokolwiek co się zserializuje do jsona. Ale jeśli Ci wygodniej używać thymeleafu do szybkiego testowania ręcznego czy działa to używaj. Ważne jest to, żeby móc zmienić łatwo kontrolery nie zmieniając nic w samej logice
  4. Trochę zależy co to if robi. Jeśli jakąś logikę, to nie powinno być  w kontrolerze. Jeśli coś z wyświetlaniem (np jakies mapowanie optionala) to raczej może być
  5. Nie używaj setterów. Jest pewnym pytaniem czy robić samowalidujące się obiekty, czy osobno walidatory, ale jeśli obiekt jest już stworzony to powinien być poprawny, a nie, że gdzieś sobie jeszcze czeka na jakiegoś settera, żeby móc życ.

Co do samego projektu. Nie mam teraz trochę czasu czytać i to co powiem dla niektórych jest kontrowersyjne ale zrobiłbym inny podział na pakiety. Nie per warstwa (myślę, że osoby domyślą się że obiekt który ma w nazwie service (swoją drogą ta nazwa nic nie mówi) jest serwisem i nie musi dodatkowo leżeć w pakiecie service) a per funkcjonalność. Czyli np rzeczy z użytownikami w user. Można wtedy łatwiej i więcej używać dostępu pakietowego, a nie robić wszystko publiczne

Jeszcze taka ogólna zasada. Najlepiej pisać tak, żeby kod domenowy nie był zależny od frameworka
-edit 2
Masz różne formatowanie w kodzie, podepnij sobię w intellij automatyczne formatowanie przed commitem. A, i po co Ci interface do service skoro jest tylko jedna implementacja?

1

No to z tego co zauwazyłem to:

1

Mamy dwa serwisy: FooService i BooService. Co w sytuacji, gdy Foo i Boo są zupełnie innymi (tzn Foo i Boo są osobnymi agregatami [o ile dobrze zrozumiałem czym są agregaty]) klasami @Entity, więc w FooService powinno być np. FooRepository a w BooService - BooRepository. Chciałbym wykorzystać obiekt klasy Boo pobierając go z bazy danych w FooService? Czy utworzenie BooRepository w FooService jest błędem (ze względu na czystość kodu)? Jeżeli nie, to jak uzyskać komunikację serwisów? Jak sprawić, żeby FooService "poprosiło" BooService, żeby ten "poprosił" BooRepository żeby pobrał coś z bazy danych?

Najlepiej nie mieszac pojęć encja/agregat z adnotacją @Entity. Pierwsze dwa mają znaczenie domenowe, drugie przy zapisywaniu (persystowaniu?) obiektów do jakiejś bazy. Zatem zakładam, że pytasz o agregaty więc odpowiedź brzmi to zależy, ale generalnie nie ma przeciwskazań. Najlepiej jakbyś robił moduły i wystawiał api przez np. FooFacade i BooFacade. Jeżeli Foo potrzebuje czegoś z Boo to jedyną publiczną klasą będzie BooFacada i ją możesz wstrzyknąć do Foo i korzystać z jej API.

Uwagi:

  • formatowanie, za dużo pustych linii.
  • testIfActiveIsOppositeToDisctive to nazwa testu u ciebie. Polecam konwencje should, czyli w tym wypadku shouldReturnOppositeStatus
  • testy mógłbyś lepiej sformatowac, ogółem poczytaj tez o BDD, chociażby mozna wproadzić sekcję given/when/then żeby czytając wiedział co jest ustawianiem testów, co akcją, a co weryfikacją
  • Offer - w testach masz pomocniczą metodę do przygotowania tego obiektu - bardzo dobrze. Złe jest, że w tej metodzie ustawiasz pola przez settery - po co ci one? Korzystaj z konstruktorów, dzięki temu od razu stworzysz obiekt pełnoprawny obiekt, zamiast obiektu którego pola to same nulla, bo dopiero zaraz ktoś będzie je ustawiał setterem
  • Offer może mieć metodę to toDto albo OfferDto może mieć metodę fromOffer i wtedy łatwo ci będzie tworzyć zmappowane instancje
  • w pakiecie domain spodziewałbym się klas domenowych, a nie klas oznaczonych jako @Entity. Generalnie tym się nie przejmuj do końca, jak dla mnie to jest ostatnia rzecz, której się musisz nauczyć, najważniejsze to najpierw ogarnąć dobrze OOP/SOLID, wtedy DDD przyjdzie z łatwością
  • SimpleStatusFactory - niepotrzebna klasa, niepotrzebna abstrakcja. Tak naprawdę mogło to być w enumie status, nie ma tam skomplikowanej logiki tylko parsowanie stringów. Po drugie jak chcesz np. utworzyć enuma ze stringa i ktoś podał wartość, której nie da się sparsować to IMHO lepiej rzucić wyjątkiem niż zwrócić nulla. A no i enum ma metodę valueOf
  • private USD USD; zmienne nazywa się wykorzystując konwencję camelCase

Polecam ci książkę clean code r.martina

0

@Interpod będę odpowiadał po kolei:

  1. Rozważę to. Słyszałem, że gradle wypiera Mavena więc w projekcie będzie to jak najbardziej na plus.
  2. Nie wiedziałem, że wersję można pobrać z properties - będę to wykorzystywał.
  3. Czemu konstruktor na końcu klasy? Powiem szczerze, że nie wiem. Tak samo nie wiem, czemu tam się @Autowired znalazło, bo Spring5 automatycznie dodaje adnotację @Autowired przy konstruktorze.
  4. Lomboka pobrałem tylko dlatego, że w jednym kursie był. Natomiast sama idea - jak najbardziej mi się podoba, tylko słyszałem, że mogą pojawić się problemy gdy IDE nie obsługuje go, więc wolę mieć zwykłe settery i gettery, nawet jeśli w nich logiki nie ma i uniknąć możliwości wystąpienia błędów.
    5.Na początku, metoda z AdminBootstrap - fillDatabase() była w AdminService i pod jednym z endpointów które były w AdminController. Testując ręcznie dodawanie ofert, zawsze musiałem jako pierwsze uzupełnić bazę. Zrobiło się to męczące,więc postanowiłem, żeby przy odpaleniu aplikacji baza danych sama się uzupełniła. A AdminController i wszystko co w nim, to "śmieci" pozostałe po refaktoryzacji.
  5. W jakim sensie miaszam UI z Backendem? Pytam serio, bo może coś źle zrobiłem a chciałbym się dowiedzieć co.
  6. Jako, że dużo commitów nie robiłem (uczyłem się też Gita przy okazji) - to stwierdziłem, że message nie jest mi jakoś potrzebny. Potem dopiero się zacząłem orientować, że bez sensu nazywam commity (jedna z przyczyn, z których chcę przestać tworzyć ten projekt i zabrać się za nowy)
  7. Co jest nie tak, w tworzeniu kontrolera odpowiedzialnego za stronę główną?
  8. Wiedziałem, że można to łatwiej zrobić. Nie pomyślałem o tym.
  9. Tutaj akurat wiem, że istnieją w Springu profile i chciałem uprzednio się przygotować (na wypadek gdybym chciał rozwijać projekt (na jakąś pracę inżynierską czy coś) i dodać coś jak internacjonalizację) wyodrębniając te metody, które zwracałyby coś co potem dodałbym do Modelu w Controlerze. Teraz wiem, że rozwijać tego nie będę.
  10. Testuję funkcjonalności springa, bo tak jak mówiłem - dopiero się go uczę (tak samo jak i testowania) więc dobrze jest, przynajmniej w moim mniemaniu, testować jak najwięcej (żeby się nauczyć pisać testy). W dodatku, nie jestem jeszcze przekonany pisząc metody w Jpql, czy rzeczywiście zwracają to co chcę, żeby zwracały - więc uznałem, że będzie to ok.

@danek :
2. W sytuacji, gdy np chcemy dodać ofertę na giełdzie, dla danej kryptowaluty to po potrzebuję pobrać z bazy danych zarówno wszystkie oferty dotyczące tej kryptowaluty (z UserRepository - jako że Lista ofert jest polem User, albo z OfferRepository) - jeżeli w DTO mamy idKryptowaluty zamiast Kryptowaluty i wtedy bym pobierał z CryptoCurrencyRepository tą walutę korzystając z Id, żeby móc DTO przekonwertować do obiektu bazodanowego i zapisać go w bazie danych.
3. BigInteger nie zapewni mi przechowywania liczb zmiennoprzecinkowych. No chyba, że korzystając z BigIntegera, przesuwałbym go o x miejsc w prawo (x - ilość miejsc po przecinku).
4. Czyli przy używaniu Thymyleafa, zwracanie zwykłego Stringa jest dopuszczalne? Nie muszę się martwić, że nie zwracam jakiegoś HttpEntity?
5. If zarządza wybraniem strony. Np jeśli by się okazało, że użytkownik nie byłby zalogowany - !sessionVariables.optionalUser().isPresent() - wyświetl (zwróć templatke) o nazwie "userNotLogged", jeżeli jest wyświetl "User".
6. No tak, ale co w przypadku gdy nie mamy żadnej walidacji w seterach, a ktoś nowy przyjdzie i będzie używał setterów do ustawiania pól? Nawet w sytuacji, gdy pole jest opcjonalne (nie wymagane w konstruktorze) a musi spełniać jakieś określone warunki?
Pakiety per funkcjonalność brzmią fajnie, oraz dostęp pakietowy. Tylko czy, np jeżeli posiadamy @Entity User z jakimiś polami, to nie lepiej stworzyć jakąś osobną klasę która będzie odpowiedzialna za dostęp do Usera? Np. miałaby metodę setName(user, name) gdzie najpierw by walidowała name, a potem jeżeli przejdzie walidację ustawiała dla user to name?

Edit: Dziękuje Panom, za tak szybką odpowiedź :)

0
  1. Service mogą normalnie mieć referencje do innych service, nie ma w tym raczej nic złego (o ile tego nie jest za dużo)
  2. Chodziło mi o BigDecimal, sorry ;) to już ma przecinki
  3. Trochę nie wiem, bo nie używam
  4. jw, nie umiem na to odpowiedzieć
  5. wywal settery, nikt ich nie użyje :D Jesli będziesz bawił się tylko niemutowalnymi obiektami (co jest zalecane) to odpadnie Ci ten problem. Po prostu stworzysz nowy obiekt ze wszystkim już

U siebie w projekcie często mam osbone klasy @Entity oraz te które mają jakąś logikę potrzebną. Jak chcesz, to możesz na własne ryzyko sobie poczytać mój projekt, acz jest to trochę moja piaskownica do walidowania pomysłów, więc ostrożnie z kopiowaniem rozwiązań ;)
https://github.com/krasnoludkolo/ebet2

0

@hcubyc :

  1. Tutaj duże znaczenie ma pakietowanie klas. Jak lepiej pakietować: per warstwa (serwisy, controlery itd), czy per funkcjinalność (User, Offer)? Jak korzystać z fasad? Tzn klasa nie może być ani private, ani protected. Jak uniemożliwić dostęp do danych klas? Czy wystarczy tworząc wszystkie metody, pola korzystać z klauzul private i protected zamiast public?
  2. Puste linie, pojawiają się w sytuacji gdy stworzyłem jakąś metodę, którą uznałem potem że jest niepotrzebna i ją usunąłem.
  3. Czyli zwykłe komentarze //given, //when, //then? Zawsze się zastanawiałem, po co to jest w kodzie u innych osób. Teraz wiem, że jednak ma znaczenie. Myślałem też, że miało to związek z testowaniem aplikacji z "zewnątrz" (jakimiś programami zewnętrznymi).
  4. Czyli utworzyć konstruktor domyślny protected (żeby Hibernate był zadowolony) a pola które są wymagane umieścić w konstruktorze? Potem w konstruktorach używać setterów. Tylko, czy te settery mogą mieć jakieś walidacje? Czy walidować za pomocą jakiejś Fasady?
  5. Czy mapowaniem OfferDto i Offer nie powinna zająć się inna klasa? Według Solid, każda klasa powinna mieć jedną odpowiedzialność, a mapowanie według mnie powinienem zostawić Mapperom.
  6. Tak, wiem o camelCase. Tutaj "USD" jest z dużych liter, bo użyłem generatora który z postaci Jsonowej, utworzył mi klasy. W Jsonie było USD, więc generator utworzył mi to z dużych liter, potem już zapomniałem to zmienić na camelCase.
    Dużo słyszałem o clean code, jednak wzięcie teraz do ręki kolejnej książki - nie widzi mi się to. Ledwo co ogarniam podstawy Springa, uczę się testowania to jeszcze kolejną rzecz musiałbym jednocześnie przerabiać. Na pewno przeczytam.
    A jeszcze jedno: jak sobie poradzić z nawarstwianiem się pól w klasie? O tą klasę mi np chodzi: https://github.com/KamLar/market-simulator/blob/master/src/main/java/larysz/kamil/cryptoapp/services/CryptoCurrencyServiceImpl.java . Zrobiłem już klasę opakowującą JsonGetter - tworzącą Stringa reprezentującego jsona, z danej strony.(wiem, zła nazwa) i JsonParser i nazwałem ją JsonOperator (zła nazwa), ale jednak wciąż nawarstwiają mi się pola: potrzebowałem OfferRepository, żeby pobierać oferty (endpoint jest w OfferController), CryptoCurrencyRepository żeby pobierać z bazy danych obiekty które ustawię jako pole Oferty (coś jak Enumy, tylko jest ich na tyle dużo że łatwiej było zapisać to w bazie danych i pobierać je gdy będzie potrzeba), sessionVariables żeby mieć mechanizm logowania i przypisywanie ofert do zalogowanego użytkownika, UserMappera do dodawania do modelu dtosów, a JsonOperatora do wyświetlania danych, a aplikacja na razie mało co robi.
    /----/
    No tak. tylko jak wywalę settery (bez Lomboka) to o ile dobrze wiem, Hibernate nie będzie wstanie utworzyć obiektów bo korzysta właśnie z getterów i setterów.
0
  1. Per funkcjonalność. Fasada ma API do tego co robi dany moduł, tak naprawdę fasada tylko deleguje do odpowiednich serwisów/agregatów praca i waliduje to co do niej przychodzi (argumenty metod fasady). Klasy w pakiecie powinny byc package scoped i metody serwisów też i tylko fasada jest klasą publiczną i ma publiczne metody.
  2. To usuń też puste linie
  3. Tak zwykłe komentarze lub jeżeli narzędzie to wspiera to korzystasz z narzędzia typu spock, ale mockito też chyba miało coś takiego
  4. Utworzyć konstruktor, hibernate powinien chyba dać radę z prywatnym, ale potrzebuje package scoped żeby inne klasy mogły tworzyc instancje. Nie ma wtedy w ogóle setterów, w konstruktorze przypisujesz pola jak w setterach. Walidacja podstawowa jest też w konstruktorze. Fasada waliduje tylko to co do niej przychodzi z zewnątrz

Dużo słyszałem o clean code, jednak wzięcie teraz do ręki kolejnej książki - nie widzi mi się to. Ledwo co ogarniam podstawy Springa, uczę się testowania to jeszcze kolejną rzecz musiałbym jednocześnie przerabiać. Na pewno przeczytam.

Clean code jest ważniejszy od springa, bo jest uniwersalny pomiędzy językami/frameworkami/etc. Spring jest na tyle duży, że człowiek nie jest w stanie go opanować chociaż w 50% (wszystkie moduły np. spring cloud etc).

A jeszcze jedno: jak sobie poradzić z nawarstwianiem się pól w klasie

Tragedii nie ma, ale najprostsze co możesz robić to rozbić klasę na mniejsze

Hibernate nie będzie wstanie utworzyć obiektów bo korzysta właśnie z getterów i setterów.

Potrafi korzystać tez z pól, a nawet gdyby nie potrafił to po to się rozdziela encje hibernate od encji domenowych żebys nie skończył z klasą domenową z setterami

Jeszcze co do rozbijania klas na mniejsze to w tym serwisie (nie rozumiem po co interfejs i jedna jedyna implementacja) masz np. metodę addOffer. Możesz zrobić klasę np. OfferAdder, OfferCreator lub OfferFactory i wynieść tę logikę z tego serwisu. Wtedy jak wyniesiesz inne funkcjonalności to już będziesz krok bliżej do fasady - czyli takiej właśnie klasy, która deleguje poszczególne funkcjonalności do innych klas, w których każda ma jedno określone zadanie.

I jeszcze jedno - w serwisach czy ogółem klasach realizujących logikę biznesową nie ma miejsca na jakieś jsony, xmle czy cokolwiek co ma związek z infrastrukturą/narzędziem/technologią. To kontroler jest odpowiedzialny za to, bo jezeli nagle dostaniesz potrzebę pracy z xmlami to będzię gnój, bo będziesz musiał modyfikować logikę serwisu.

0

Myślałem bardziej przyszłościowo tworząc interfejs i (na razie jedną implementację) bo miałem na uwadze Springowe Profile i w sytuacji, gdy chciałbym móc potem w razie konieczności, nie zmieniając kodu w kontrolerach, wykorzystać Springa żeby wstrzyknął mi odpowiednią implementację serwisu. Tak samo jak np UserMapper czy OfferMapper na razie mam jedną implementację i jeden interfejs, chociaż nie wykluczałem tego, że byłby np. język Polski.

Jsona parsuje dlatego, bo pobieram z zewnętrznego API dane. W sumie jak teraz na to spojrzeć, to wykonuje mniej-więcej coś takiego: pobieram Jsona z zewnętrznego api -> Parsuje to na javowe obiekty -> Dodaję do Modelu -> W thymyleaf pobieram to z modelu. Ewentualnie, pobieram dane z zewnętrznego Api ponieważ, są tam dane które potem wykorzystuję w aplikacji.

Czyli FooFacade jako pole ma obiekt klasy FooService i jeśli BooService coś potrzebuje "związanego" z Foo to prosi o to fasadę? Czy w serwisie mogą być gettery, żeby metod które są wywoływane z zewnątrz (a nie będą nigdy wywoływane w serwisie) nie tworzyć w Serwisie a w Fasadzie?

Czy sposób z tworzeniem zmiennych sesyjnych jest dobry? Tak jak tutaj https://github.com/KamLar/market-simulator/blob/master/src/main/java/larysz/kamil/cryptoapp/session/SessionVariables.java , tylko z wyjątkiem takim, że zamiast User, byłby Optional<User>? I w zależności czy logowanie się powiedzie, to przypisałbym do tego pola zalogowanego Usera pobranego z DB? I w tym przypadku obiekt klasy SessionVariables (zmienić nazwę można byłoby na UserSession czy cokolwiek) byłby polem obiektu klasy UserService i jeżeli cokolwiek z zewnątrz by chciało zmienić coś to przez Fasadę?

Właśnie rozbijanie klas na drobniejsze będzie powodowało, że np. mój OfferService będzie miał jeszcze więcej pól (zależał od obiektów jakichś klas - w tym przypadku OfferCreator). Problemem nie jest sam fakt, że będzie coraz więcej zależności (bo Spring i tak mi to sam wstrzyknie według konfiguracji) tylko to, że słyszałem, że jeżeli klasa zależy od dużej ilości klas, to jest źle zaprojektowana ta klasa.

0

Myślałem bardziej przyszłościowo tworząc interfejs i (na razie jedną implementację) bo miałem na uwadze Springowe Profile i w sytuacji, gdy chciałbym móc potem w razie konieczności, nie zmieniając kodu w kontrolerach, wykorzystać Springa żeby wstrzyknął mi odpowiednią implementację serwisu.

Może i ja się mylę, ale klasy biznesowe zawsze mają konkretną implementację, zawsze robić coś w jeden sposó (do tego dochodzą np. strategie, ale to nie ten przypadek). Klasy biznesowe powinny być najstabilniejszym elementem systemu, logika powinna być realizowana zawsze w ten sam sposób czy to spring, play czy C++. Jaką inną implementację chcesz tam mieć?

Tak samo jak np UserMapper czy OfferMapper na razie mam jedną implementację i jeden interfejs, chociaż nie wykluczałem tego, że byłby np. język Polski.

Daj przykład w kodzie, bo na całą apkę nie patrzyłem.

Jsona parsuje dlatego, bo pobieram z zewnętrznego API dane.

No....to skoro masz zależność na zewnętrzny serwis to podpowiem ci jak można to zrealizować. W pakiecie danej funkcjonalności robisz sobie interfejs z API, które ty potrzebujesz, czyli co ci rozwiązuje ten serwis. Np. ExchangeRatesProvider, który daje metodę exchangeRate(from, to). Implementujesz sobie taki interfejs gdzieś poza pakietem tej funkcjonalności i tam masz całą logikę odpowiedzialną za komunikację z tym serwisem, a także mapowanie jsonów na obiekty. Podpowiedź - interfejs opiera się na obiektach domenowych, czyli jakieś DTOsy znajdujące się w tym samym pakiecie. W tym momencie twój serwis/klasa biznesowa jest niezależna tak naprawdę od implementacji czyli np. z którego zewnętrznego serwisu pobierasz te informacje czy w jakim formacie, poza tym nie masz żadnej klasy, która by dotykała infrastruktury (pomaga w testach, także zwiększa 'utrzymalność' kodu).

Czyli FooFacade jako pole ma obiekt klasy FooService i jeśli BooService coś potrzebuje "związanego" z Foo to prosi o to fasadę? Czy w serwisie mogą być gettery, żeby metod które są wywoływane z zewnątrz (a nie będą nigdy wywoływane w serwisie) nie tworzyć w Serwisie a w Fasadzie?

Generalnei tak, tylko to nie bedzie FooService, bo FooService jako nazwa nie wiele mówi i z pewnością za wiele robi, więc taki FooFacade bedzie miał jako pola kilka takich serwisów i do nich delegował. Drugiego za bardzo nie rozumiem, podaj przykład.

Właśnie rozbijanie klas na drobniejsze będzie powodowało, że np. mój OfferService będzie miał jeszcze więcej pól (zależał od obiektów jakichś klas - w tym przypadku OfferCreator). Problemem nie jest sam fakt, że będzie coraz więcej zależności (bo Spring i tak mi to sam wstrzyknie według konfiguracji) tylko to, że słyszałem, że jeżeli klasa zależy od dużej ilości klas, to jest źle zaprojektowana ta klasa.

Tak tylko twój OfferService będzie miał zależności tylko do klas pokroju OfferCreator - a one wewnątrz będą miały zależności na jakieś repozytoria etc. Tym samym OfferService stanie się fasadą ;). Generalnie tak, jeżeli klasa zależy od zbyt wielu rzeczy to jest to błąd projektowy, natomiast w przypadku fasad jest to nieuniknione, ale to tylko delegowanie więc nie ma w tym nic złego. Oczywiście czasem moduł jest zbyt duży i zbyt wiele robi, ale to już inna dyskusja.

Czy sposób z tworzeniem zmiennych sesyjnych jest dobry?

Sesje są cięzkie, nie wiem jak one działają w springu czy są bezpieczne dla wątków (podejrzewam, że są trzymane w thread localach czyli to zależy), wiec tutaj musisz sobie sam doczytać, generalnie bardziej skalowalne jest trzymanie informacji po stronie klienta w ciastku czy w inny sposób i wysyłanie np. tokena z każdym requestem.

0

Czyli tworząc serwisy, niepotrzebne są interfejsy? Muszę się jeszcze nauczyć rozróżniać klas biznesowych od pozostałych.

Pomijając już fakt, że robione na setterach ale tutaj przykład https://github.com/KamLar/market-simulator/blob/master/src/main/java/larysz/kamil/cryptoapp/mappers/SimpleUserMapper.java o który mi chodziło - interfejs z jedną implementacją i zamysłem o dodaniu następnych. Według mnie prościej tak, niż potem wydzielać odpowiednie metody i refaktoryzować kod.

Tak, u mnie serwisy bardzo dużo robią. Zawsze korzystam z nich jako pierwszej warstwy łączącej warstwę kontrolera z logiką aplikacji, wywołują odpowiednie metody dla danego endpointu. I myślałem, żeby Fasada była zupełnie innym obiektem który by miał zależność od serwisu, a serwis od fasady. Wtedy serwis by bezpośrednio komunikował się z kontrolerem i np gdybym chciał utworzyć jakąś ofertę to:
OfferController komunikuje się z OfferService przekazując Dto z polem id-> OfferService komunikuje się z OfferFacade -> OfferFacade komunikuje się z CryptoCurrencyFacade -> Jeżeli w CryptoCurrencyService byłaby metoda która w jakiś sposób pobierała z bazy danych CryptoCurrency o podanym id -> OfferService komunikuje się z OfferCreatorem podając mu pobrany obiekt CryptoCurrency oraz wykonuje pozostałe akcje (typowe dla giełdy - autozakup itd). Tak miałaby mniej-więcej wyglądać pobranie czegoś za pomocą fasady?

Tylko pytanie, czy implementowanie interfejsu znajdującego się w jednym pakiecie, wystarczy żeby klasa implementująca go miała dostęp do klas, w pakiecie w którym znajduje się interfejs (jeżeli te klasy są o dostępie pakietowym)? Pytam, bo nigdy nie korzystałem z dostępu pakietowego.

Edit: Zapomnijmy o Fasadzie i Serwisie. Teraz naszła mnie myśl, że w sytuacji gdyby serwis miał dostęp pakietowy a miał jakąś logikę - tą logika byłaby bardzo ciężko testowalna, więc tak jak mówisz Serwis musiałby pełnić rolę fasady.

0

Bardzo fajny wątek. Takie pierdoły, które każdy programista powinien znać, ale jednak często się nad nimi zastanawiamy. ;)

0
Aisekai napisał(a):

OfferController komunikuje się z OfferService przekazując Dto z polem id-> OfferService komunikuje się z OfferFacade -> OfferFacade komunikuje się z CryptoCurrencyFacade -> Jeżeli w CryptoCurrencyService byłaby metoda która w jakiś sposób pobierała z bazy danych CryptoCurrency o podanym id -> OfferService komunikuje się z OfferCreatorem podając mu pobrany obiekt CryptoCurrency oraz wykonuje pozostałe akcje (typowe dla giełdy - autozakup itd). Tak miałaby mniej-więcej wyglądać pobranie czegoś za pomocą fasady?

OfferControler komunikowałby sie z OfferFacade, OfferService jeżeli by istniał to byłby niepubliczny.
Czyli OfferController komunikuję się z OfferFacade, OfferFacade komunikuj się z CryptoCurrencyFacade, nastepnie komunikacja z OfferService i OfferCreatorem.

Edit: Zapomnijmy o Fasadzie i Serwisie. Teraz naszła mnie myśl, że w sytuacji gdyby serwis miał dostęp pakietowy a miał jakąś logikę - tą logika byłaby bardzo ciężko testowalna, więc tak jak mówisz Serwis musiałby pełnić rolę fasady.]

Byłaby bardzo łatwo testowalna, wystarczy mieć testy jednostkowe w tym samym pakiecie tylko w katalogu tests, a poza tym testować też powinieneś przez fasadę, nie obchodzą cię bebechy modułu, testowanie każdej klasy osobno utrudnia później refactoring.

0

Czyli jeżeli zrobię mirror pakietów w folderze /test/ to klasy o package scope, będą dostępne w pakiecie o tej samej nazwie (te same nazwy i hierarchię będą miały parent-packages)?
Jeżeli tak, to spodobał mi się pomysł utworzenia serwisu jako pierwszej warstwy (gettery dla pól takich jak np OfferCreator), potem Fasada i w Serwisie będę miał logikę która będzie wykorzystywana przez kontroler, a w Fasadzie - metody które będą wykorzystywane przez Api na zewnątrz pakietu. Jest to poprawne?

0

Czyli jeżeli zrobię mirror pakietów w folderze /test/ to klasy o package scope, będą dostępne w pakiecie o tej samej nazwie (te same nazwy i hierarchię będą miały parent-packages)?

Tak (sprawdzenie tego to chwila moment jakby co ;))

Jeżeli tak, to spodobał mi się pomysł utworzenia serwisu jako pierwszej warstwy (gettery dla pól takich jak np OfferCreator), potem Fasada i w Serwisie będę miał logikę która będzie wykorzystywana przez kontroler, a w Fasadzie - metody które będą wykorzystywane przez Api na zewnątrz pakietu. Jest to poprawne?

Z tego co rozumiem to tak. Jakub Nabardalik miał o tym prezentację pod nazwą hiding your shit albo jakoś tak, generalnie polecam. Jak będziesz chciał przykład to moge później podrzucić

0

Nie miałem przy sobie laptopa, żeby to sprawdzić - ale w takim razie, wygląda to bardzo fajnie że klasy package-scope w tych samych pakietach tylko różniące się tym, ze jedne są w katalogu /test/ a drugie w /src/ mają do siebie dostęp.
Przykład jak najbardziej poproszę :) a tą prezentację obejrzę, bo ten pomysł bardzo mi się podoba pomysł, żeby rozdzielić metody - te które są wywoływane wewnątrz pakietu do np. Serwisu, a te z zewnątrz - do fasady (jeżeli nie ma ich w serwisie, jeżeli jest oddelegować do serwisu).

0

Bardzo merytoryczny film, zmienił podejście co do package-scope, spróbuje je wykorzystać w chwili gdy będę ten mój projekt pisał jeszcze raz, do dziedziczenia oraz uświadomił odnośnie zmiany podejścia z pakietowaniem klas. W sumie jak na to patrzę teraz, dużo prościej będzie skupić się na testach jednostkowych fasady, niż testować pozostałe klasy osobno.
Ostatnie pytanie: czy warto zostawić ten projekt na githubie, wiedząc że bardzo dużo błędów w nim zostało zrobionych i stworzyć nowe repozytorium (na nowo pisząc aplikację o tej samej tematyce) w którym będę się starał pisać bardziej "czysto"? Czy po prostu je usunąć?

0

Jak najbardziej. Generalnie jak będziesz się komuś chwalić to ktoś będzie widział najnowszą wersję. Oczywiście może przejrzeć historię, ale każdy się kiedyś uczył, więc ja bym wolal widzieć nawet, jaki ktoś zrobił progres, niż po prostu cały projekt wrzucony w dwóch commitach i elo

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