Book Rentals - Spring Boot - ocena

Odpowiedz Nowy wątek
2018-12-29 16:41
0

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

Pozostało 580 znaków

2018-12-31 09:50
1

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:

.


edytowany 4x, ostatnio: Patryk27, 2018-12-31 10:18
Pokaż pozostałe 6 komentarzy
Ad 1: jeden rabin powie tak, inny inaczej. U siebie trzymam kontrolery w przestrzeni Application, lecz nikogo nie zmuszam ;-) Zrób tak jak CI się wydaje ładniej / sensowniej i najwyżej zobaczysz czy też to czujesz. - Patryk27 2018-12-31 19:00
Szczerze powiedziawszy to najsensowniej dla mnie był podział, który miałem na samym początku, bo kontrolery miałem w pakunku kontrolery, serwisy w serwisach etc. :P Aczkolwiek może zrobie, żę kontrolery w kontrolerach a reszta tak jak mam teraz, czyli: w bookrentals klasy dotyczące wypożyczeń, w book klasy dotyczące samych w sobie książek etc. Dobry pomysł? - must 2018-12-31 19:29
Brzmi oki. Twój początkowy podział może zdawać się mieć sens w przypadku mniejszych aplikacji, lecz staje się cholernie uciążliwy z czasem - zwyczajowo implementuje się / poprawia rzeczy zgodnie z funkcjonalnościami (np. muszę mieć możliwość edycji załączników), a nie wzorcami architektonicznymi (np. muszę mieć koniecznie w tym katalogu repozytorium), więc dzięki układowi zgodnie z domeną nie musisz co chwilę przeskakiwać z katalogu do katalogu, bo wszystko masz w jednym miejscu. W temacie: Jaki to wzorzec architektoniczny? - Patryk27 2018-12-31 19:34
@Patryk27: dzięki, jakieś konkretne jeszcze uwagi co do samego kodu? - must 2018-12-31 19:37
Wydaje mi się, że reszta jest okay :-) - Patryk27 2018-12-31 19:38

Pozostało 580 znaków

2018-12-31 10:30
0

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?

edytowany 3x, ostatnio: furious programming, 2019-01-01 23:42
Nie cytuj całego postu ;-) - Patryk27 2018-12-31 10:48

Pozostało 580 znaków

2018-12-31 10:57

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.


edytowany 5x, ostatnio: Patryk27, 2018-12-31 10:57

Pozostało 580 znaków

2019-01-02 22:38
0

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.

Pozostało 580 znaków

2019-01-02 22:46
1

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.

edytowany 1x, ostatnio: Patryk27, 2019-01-02 22:48

Pozostało 580 znaków

2019-01-02 23:05
0

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ć?

pomyłka, nie metodę tylko klasę - must 2019-01-03 09:08

Pozostało 580 znaków

2019-01-03 08:17
0

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


Pozostało 580 znaków

2019-01-03 19:54
0

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

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

Pozostało 580 znaków

2019-01-03 20:12
Szalony Polityk
0

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.

edytowany 2x, ostatnio: Patryk27, 2019-01-03 20:36

Pozostało 580 znaków

2019-01-03 20:16
0
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.

edytowany 2x, ostatnio: must, 2019-01-03 20:21

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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