Book Rentals - Spring Boot - ocena

Cześć. Napisałem swoją pierwsza webową apke w SpringBoocie.
Użyłem PostgreSQL.
Jak to wypożyczalnia składa się z:

• możliwości wypożyczania i zwracania książek z wieloma zabezpieczeniami
• typowych CRUD'owych operacji
• systemu kar pieniężnych

Za dużo logiki nie ma. Chodzi mi bardziej o jakość kodu, co poprawić, co usunąć, co zmienić, jakieś uwagi odnośnie testów.
Może coś dodać do tego? Szczerze powiedziawszy, to nie wiedziałem jaki "ficzer" mógłbym tutaj jeszcze wcisnąć.

I pytanie na koniec, na czym się skupić po tym.

Link do GH: bookrentals

IMO tworzysz ogromną liczbę mocków w sytuacji, gdzie tak naprawdę nie potrzebujesz ani jednego.

Zamiast pisania (w pseudokodzie):

class AccountRepository {
  /* ... */

  Account findById(Id id) {
    return this.database.findById(Account, id);
  }
}

Możesz podejść do tego bardziej obiektowo - z wykorzystaniem interfejsów;

interface AccountRepository {
  Account findById(Id id);
}

class MySQLAccountRepository implements AccountRepository {
  /* ... */

  Account findById(Id id) {
    return this.database.findById(Account, id);
  }
}

class InMemoryAccountRepository implements AccountRepository {
  accounts: Map<Id, Account>;

  Account findById(Id id) {
    return this.accounts.get(id);
  }
}

Jeśli we wszystkich serwisach będziesz polegał na interfejsie AccountRepository, będziesz w stanie w produkcyjnej aplikacji przekazać implementację MySQLAccountRepository, a w testach InMemoryAccountRepository - dzięki temu obejdziesz się z zerową liczbą mocków :-)

Zmieni to zupełnie Twoje podejście do testów i zaczną nabierać dzięki temu większego sensu - testy oparte o mocki często wyglądają tak: zakładając, że wszystko jest ok, odpal mocka, który potwierdzi, że wszystko jest ok, przez co pokrywają małą ilość rzeczywistego (Twojego) kodu.

Dodatkowo warto byłoby zmienić układ katalogów - wydaje mi się, że @jarekr000000 nazwał kiedyś to architekturą nieśmiałą, co zaskakująco dobrze oddaje tę sytuację: patrzę na katalogi, widzę service, repository czy configuration... i nie wiem co z czym się łączy, nie wiem o co w aplikacji chodzi.

Zamiast katalogować aplikację wzorcami projektowymi, znacznie przejrzyściej wychodzi układ względem domeny (tego, co aplikacja robi):

bookrental/

  Account/
    Account.java
    AccountRepository.java
    MySQLAccountRepository.java
    InMemoryAccountRepository.java
  
  Book/
    Book.java
    BookRepository.java

...

Filmik w temacie: .

O ile katalogowanie rozumiem i to zrobię, tak pierwszą poradę nie wiem jak zrobić.

1. Rozszerzam interfejs o klasę CrudRepository i jakaś inna klasa ma ten interfejs jeszcze implementować?
2. Czym się różni w takim razie MySQLAccountRepository od InMemoryAccountRepository?
3. Mam rozumieć, że w testach nie mockuje żadnej klasy tylko używam już gotowego InMemory... ?

Różnica pomiędzy tym co mam teraz polega tylko na dodatkowej klasie InMemoryAccountRepository, prawda?

Rozszerzam interfejs o klasę CrudRepository i jakaś inna klasa ma ten interfejs jeszcze implementować?

Nie możesz rozszerzać interfejsu o klasę, zatem nie do końca rozumiem pytanie.

Czym się różni w takim razie MySQLAccountRepository od InMemoryAccountRepository?

Sposobem działania / zachowaniem - MySQLAccountRepository pobiera dane z bazy danych (stąd wewnątrz masz this.database.findById), podczas gdy InMemoryAccountRepository działa na wewnętrznej hashmapie (dlatego tam z kolei jest this.accounts.get).

Mógłbyś nawet mieć np. FileAccountRepository oparte o plik CSV i Twoje wszystkie serwisy nadal działałyby poprawnie - dzięki interfejsowi nie ograniczasz swojej aplikacji tylko do MySQLa, możesz szybciej prototypować (możesz zrobić kilka pustych interfejsów i zająć się implementacją później) no i dochodzą też porządne testy.

Znam kilka osób, które oparły prototypy aplikacji właśnie o takie FileCośtamRepository i dopiero później (po etapie prototypowania) przeskoczyli na prawdziwą bazę danych - przeskok był bezproblematyczny właśnie dlatego, że wszystkie serwisy aplikacji działały na abstrakcji (interfejsie CośtamRepository) i nie obchodziło ich to czy pod spodem działa MySQL, czy też ręcznie obrabiane pliki.

Fachowo mówimy w tej chwili o separation of concerns i dependency inversion principle.

Mam rozumieć, że w testach nie mockuje żadnej klasy tylko używam już gotowego InMemory... ?

Tak, dokładnie.

Różnica pomiędzy tym co mam teraz polega tylko na dodatkowej klasie InMemoryAccountRepository, prawda?

Pojawia się dodatkowy interfejs (AccountRepository), pojawiają się jego dwie implementacje (InMemoryAccountRepository + MySQLAccountRepository) no i wywalasz wszystkie mocki z testów.

Okej, rozumiem całą ideę. Nie do końca wiem jednak jak to zrobić. Teraz mam np:
public interface BookRepository extends CrudRepository<Book, Integer> czyli korzystam z wbudowanych już metod crudowych. Ty u góry podałeś bym stworzył całkiem nowy interfejs bez rozszerzania tego. Skąd teraz wziąć te metody? Samemu implementować?

Account findById(Id id) {
    return this.database.findById(Account, id);
  }

czym jest to database? W sensie jak wyglądała by inicjalizacja tego.

Rozszerzanie takiego CrudRepository jest swego rodzaju anty-patternem - interfejs powinien być sprecyzowany i zawierać tylko metody, które potrzebujesz.

Co nie znaczy oczywiście, że trzeba wynajdywać koło od nowa - spójrz na coś takiego:

class MySQLAccountRepository implements AccountRepository {
  private CrudRepository<Account, Integer> repository;

  // tutaj konstruktor

  public Account findById(Id id) {
    return this.repository.findById(id); // czy jak tam ta metoda się nazywa
  }
}

Wykorzystanie kompozycji zamiast dziedziczenia może początkowo sprawiać wrażenie zachęcającego do duplikowania istniejącego już kodu (przecież CrudRepository już daje mi metodę Foo!), lecz dzięki temu:

1. Nasz interfejs / klasa nie eksponuje na zewnątrz metod, których nie potrzebujemy w aplikacji - taki CrudRepository przykładowo wystawia na świat np. findAll(), saveAll() czy count(), które mogą być przydatne podczas implementacji repozytorium (tak jak w przykładzie wyżej), lecz niekoniecznie od razu wewnątrz całej aplikacji.
2. Załóżmy, że opieramy nasz interfejs AccountRepository o CrudRepository (czyli załóżmy, że mamy interface AccountRepository extends CrudRepository<...>) - w jaki sposób chcesz szybko zaimplementować te kilkanaście metod z CrudRepository w Twoim hipotetycznym InMemoryAccountRepository? Byłoby to dosyć żmudne i - mało tego - prawdopodobnie zbędne, chyba że wszystkie te metody faktycznie wykorzystujesz w aplikacji.
3. Układanie klas w hierarchie przeważnie kończy się źle długofalowo - w większości przypadków wystarczy kompozycja.
4. Można jeszcze dywagować na temat persistence ignorance, lecz jest to dosyć grząski grunt, więc jedynie zasiewam ziarno, że coś takiego też istnieje i jest nazwane.

Dobra, czyli mam zrobić coś takiego jeżeli chodzi o aplikację poza testami: gist.
Natomiast jeżeli chodzi o tę metodę InMemory, którą mam używać tylko w testach - mam tam operować tylko na hashmapie i implementować sam te metody czy w jaki sposób ma to wyglądać?

Tak, hashmapa + re-implementacja metod interfejsu tak, aby opierały się na hashmapie.

1)Jak mam obejść metody w interfejsie, które miały taką postać:

 @Query("SELECT CASE WHEN COUNT(account) > 0 THEN true ELSE false END FROM Account account WHERE account.id =:accountID")
    boolean doesAccountExistsWithGivenID(@Param("accountID") int accountID);

Bo teraz musze zaimplementować tę metodę w klasie PostgreSQLAccountRepository

1. Mógłbys zaimplementować przykładową metodę np. findAll() na hashmapie i jakby wyglądał test dla niego?

Ad.1.
Tworzysz HashMap<Integer, Account> i sprawdzasz. Co to znaczy obejść? Powinienes mieć interfejs boolean doesAccountExist(int accountID), a dopiero w jakiejś jego implementacji zrobić to Query.

Ad.2.
Pytanie, czy wyciąganie wszystkich rekordów jest dobre? Może zrób stronicowanie? Jak wyobrażasz sobie pobieranie całej bazy danych na produkcji?

Powinienes mieć interfejs z metodą boolean...
I w sumie lepiej jest użyć ConcurrentHashMap.

Szalony Polityk napisał(a):

Ad.1.
Tworzysz HashMap<Integer, Account> i sprawdzasz. Co to znaczy obejść? Powinienes mieć interfejs boolean doesAccountExist(int accountID), a dopiero w jakiejś jego implementacji zrobić to Query.

Chodziło mi o PostgreSQLAccountRepository gdzie operuje na private CrudRepository<Account, Integer> repository;
W jaki sposób mam użyć w tej klasie w metodzie doesAccountExistsWithGivenIDinstrukcje @Query("SELECT CASE WHEN COUNT(account) > 0 THEN true ELSE false END FROM Account account WHERE account.id =:accountID"), którą miałem poprzednio w interfejsie.
Mam interfejs z tą metodą. Robie tak jak napisał @Patryk27, czyli interfejs z wszystkimi metodami, które pozniej implementuje w PostgreSQLAccountRepository

Ad.2.
Pytanie, czy wyciąganie wszystkich rekordów jest dobre? Może zrób stronicowanie? Jak wyobrażasz sobie pobieranie całej bazy danych na produkcji?

Chce wyświetlić po prostu wszystkie dostępne książki. Nie wiem jak to zrobić za pomocą HashMapy i jak wyglądał by test do tego.

Ad 1: Przecie już masz taką metodę: https://docs.spring.io/spring-data/commons/docs/current/api/org/springframework/data/repository/CrudRepository.html#existsById-ID-.
Ad 2: this.hashmap.values().

Btw, staraj się skracać nazwy metod - jeśli masz interfejs AccountRepository, metoda nie musi nazywać się getAccountById - wystarczy samo getById. Podobnie nie doesAccountExistsWithGivenID, tylko raczej własnie existsByID lub pochodne.

jak wyglądał by test do tego.

Pobieranie wszystkich książek nie jest funkcjonalnością / nie zawiera żadnej logiki, zatem nie ma co w tym przypadku testować - chyba że jest to część czegoś większego (np. serwisu liczącego statystyki z wszystkich książek z bazy), no ale wtedy to testuje się całościowo to coś większego, a nie samą metodę findAll().

1. Możliwe, że jest. Nie szukałem czy są takowe to fakt, tylko sam zrobiłem. Mam wiele innych metod, które używają JPQL, dlatego wygodniej by mi było użyć @Query aniżeli się zagłębiać, które metody są już napisane a które nie.

2. Zgłoszę się w takim razie z innymi metodami, jak uda mi się zrobić tę klasę InMemory... Swoją drogą, dlaczego akurat tutaj używamy HashMapy?

3. czy to this w this.repository jest potrzebne?

Ad 1: W takim razie pewien nie jestem - musiałbyś poczytać w dokumentacji jak odpalać zapytania bez adnotacji.
Ad 2: Ponieważ dzięki temu możemy łatwo mapować id obiektu na jego faktyczną instancję - moglibyśmy używać np. tablicy, ale jak wtedy chciałbyś zaimplementować metodę delete bez zmieniania idków wszystkich następnych encji?
Ad 3: Nie, to tylko przykład.

1. Najpewniej trzeba użyć SessionFactory.
2. Czy na pewno można zainicjalizować CrudRepository, tak o? Gdy usunąłem poprzednią wersję AccountRepository która rozszerzała CrudRepository wywaliło mi błąd.:

Error starting ApplicationContext. To display the conditions report re-run your application with 'debug' enabled.
2019-01-03 23:21:06.427 ERROR 7608 --- [           main] o.s.b.d.LoggingFailureAnalysisReporter   : 

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 0 of constructor in bookrental.account.PostgreSQLAccountRepository required a bean of type 'org.springframework.data.repository.CrudRepository' that could not be found.


Action:

Consider defining a bean of type 'org.springframework.data.repository.CrudRepository' in your configuration.

AccountRepository, PostrgreSQLAccountRepository oraz AccountService wygląda tak: https://gist.github.com/must1/1929fedff64af9857362ea963818351a

Ad 2: W takim razie najwyraźniej niestety trzeba by podejść inaczej - niestety sam javowcem nie jestem, więc tutaj kończy się moja możliwość podpowiadania :-P Być może mógłbyś np. przez DI wstrzyknąć sobie entity manager i na jego podstawie budować zapytania, lecz może też da się podejść do tego lepiej.

Przy okazji co do Twojego gistu: AccountService jest słabą nazwą na klasę, bo nie wiadomo tak właściwie czym się zajmuje (a jeśli wszystkim, to też źle). Serwisy powinny być sprecyzowane (np. AccountStatisticsReporter), nie ogólne + nie potrzebujesz serwisu, którego jedynym zachowaniem jest odpalanie repo - nie ma nic złego we wrzucaniu repozytorium jako zależności do kontrolera.

Co do nazw, to pozmieniam na końcu.

Rozwiązaniem tego o co się pytałem jest:

@Repository
public interface AccountRepository implements  CrudRepository<Account, Integer>{
}

@Repository
public class PostgreSQLAccountRepository {

    private final AccountRepository repository;

    @Autowired
    public PostgreSQLAccountRepository(AccountRepository repository) {
        this.repository = repository;
    }

    public void deleteById(Integer id) {
       this.repository.deleteById(id);
    }

    public List<Account> findAll() {
        return (List<Account>) repository.findAll();
    }
   
     public boolean doesAccountExistsWithGivenID(int accountID){
       ...
     }

tylko nie wiem czy jest to dobre rozwiązanie, co sądzisz?

To nie jest rozwiązanie w ogóle:

1. Klasa PostgreSQLAccountRepository nie implementuje AccountRepository, czyli tracisz możliwość podmiany implementacji (jak chcesz teraz zaimplementować InMemoryAccountRepository i przekazać jedną bądź drugą implementację do serwisu?).
2. Twój interfejs AccountRepository przestał być wyspecjalizowany i zaczął eksportować na zewnątrz metody w stylu deleteAll(), które niekoniecznie będą Ci potrzebne.

Kilka uwag:

1. Nazwy restowych mappingow moga byc zdefiniowane na poziomie klasy zamist powtarzac je przy kazdej metodzie(zamist "books" powinno byc "book")
2. Brak zdefiniowanych interfejsow w implementacjach serwisów
3. Manualne przepisywanie kolekcji zamiast uzycia collectors
4. Serwisy nie powinny zwracac wartosci null
5. Serwisy zwracaja wartosci String zamiast zmodyfikowane objekty.
6. Mutowale klasy Entity, moze to prowadzic do trudnych do wykrycia problemow, lepiej uzyc konstruktor kopiujacy.

p_maciek napisał(a):

Kilka uwag:

1. Nazwy restowych mappingow moga byc zdefiniowane na poziomie klasy zamist powtarzac je przy kazdej metodzie(zamist "books" powinno byc "book")

Faktycznie, poprawię.

1. Brak zdefiniowanych interfejsow w implementacjach serwisów

Tak jak pisał, @Patryk27 w komentarzu, nie jest to chyba potrzebne.

1. Manualne przepisywanie kolekcji zamiast uzycia collectors

Mógłbyś podać przykład o co chodzi dokładnie, w którym miejscu?

1. Serwisy nie powinny zwracac wartosci null

Tak samo jak w 3.

1. Serwisy zwracaja wartosci String zamiast zmodyfikowane objekty.

Co złego jest w zwracaniu Stringa? W niektórych miejscach zwracam obiekty, w niektórych Stringa. Wiem też, że powinno się się zwracać HTTP STATUS. Dlaczego wg Ciebie obiekty są najlepsze?

1. Mutowale klasy Entity, moze to prowadzic do trudnych do wykrycia problemow, lepiej uzyc konstruktor kopiujacy.

Wszystkie pola są oznaczone private, mam je oznaczyć także jako final, nie udostępniać getterów ? Mógłbyś ten punkt rozwinąć?

1. Manualne przepisywanie kolekcji zamiast uzycia collectors

Mógłbyś podać przykład o co chodzi dokładnie, w którym miejscu?

public List<Account> getAllAccounts() {
List<Account> accounts = new ArrayList<>();
accountRepository.findAll().forEach(accounts::add);
return accounts;
}

1. Serwisy nie powinny zwracac wartosci null

Tak samo jak w 3.

public Book deleteBook(int id) {
Book bookToDelete = bookRepository.findById(id).orElse(null);
bookRepository.deleteById(id);
return bookToDelete;
}

1. Serwisy zwracaja wartosci String zamiast zmodyfikowane objekty.

Co złego jest w zwracaniu Stringa? W niektórych miejscach zwracam obiekty, w niektórych Stringa. Wiem też, że powinno się się zwracać HTTP STATUS. Dlaczego wg Ciebie obiekty są najlepsze?

np. bo trzeba parsować stringa aby sprawdzić co się stało.

1. Mutowale klasy Entity, moze to prowadzic do trudnych do wykrycia problemow, lepiej uzyc konstruktor kopiujacy.

Wszystkie pola są oznaczone private, mam je oznaczyć także jako final, nie udostępniać getterów ? Mógłbyś ten punkt rozwinąć?

Tzn. chodziło o to aby klasa była niemutowalna, czyli final na polach, gettery mogą być ale bez setterów.