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

2019-01-03 20:33
1

Ad 1: Przecie już masz taką metodę: https://docs.spring.io/spring[...]epository.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().


edytowany 3x, ostatnio: Patryk27, 2019-01-03 20:44

Pozostało 580 znaków

2019-01-03 21:25
0

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?

edytowany 3x, ostatnio: must, 2019-01-03 21:35

Pozostało 580 znaków

2019-01-03 21:57
0

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.


Pozostało 580 znaków

2019-01-03 23:28
0
  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

Pozostało 580 znaków

2019-01-04 08:05
0

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.


edytowany 1x, ostatnio: Patryk27, 2019-01-04 08:05

Pozostało 580 znaków

2019-01-04 14:08
0

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?

Pozostało 580 znaków

2019-01-04 14:12
0

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.

edytowany 4x, ostatnio: Patryk27, 2019-01-04 14:17
w takim razie ciężko o rozwiązanie :/ - must 2019-01-04 14:19

Pozostało 580 znaków

2019-01-08 17:28
0

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.
Pokaż pozostałe 5 komentarzy
@p_maciek: to może inaczej: co, według Ciebie, daje Ci bezwarunkowe tworzenie interfejsów do każdego serwisu? Według mnie, na przykład, nie jest to wcale pisanie zgodnie z dobrymi zasadami, bo podstawową dobrą zasadą zawsze powinno być KISS / YAGNI. Podczas pisania kodu należy kierować się czytelnością i maintenability, które znacznie maleje w momencie, kiedy w trakcie analizy kodu musisz przeciskać się przez setkę interfejsów w imię jakiejś reguły - bo trzeba tak, bo tak,i już. Been there, done that - zrezygnowałem właśnie z wymienionego w poprzednim zdaniu powodu. - Patryk27 2019-01-09 17:27
Kontynuując: IMO nie należy udawać, że jakieś moduły są niezależne - fragmenty kodu zależą od siebie i jest to całkowicie naturalne (gdyby wszystkie moduły były odrębne, stanowiłyby osobne aplikacje) - martwić powinien co najwyżej wysoki coupling (A zależy bezpośrednio od B, które zależy bezpośrednio od C, które zależy od A i D, które zależą od B, C, D i MagendoBeanFactoryProxyWrapper). Udawanie, że klasa A nie zależy od klasy B, bo przecież ona wymaga BInterface brzmi jak próba kłamania prosto w oczy swoje oraz innych programistów. - Patryk27 2019-01-09 17:31
Powinny być jak najbardziej niezależne, i po to właśnie tworzymy warstwę abstrakcji. Implementacja serwisu nie powinna byc w ogole publiczna. Odzielenie implementacji od kontraktu daje tyle, że zmiana implemetacji danego serwisu nie wpływa na moduł który z niej korzysta. Ten tak naprawdę nie powinien wiedzieć, że coś się zmienia. - p_maciek 2019-01-10 12:28
W imię tego powstały fasady, nie interfejsy (https://www.youtube.com/watch?v=ILBX9fa9aJo). Fasada daje możliwość oddzielenia wnętrzności modułu od jego implementacji (czyli osiągnięcia właśnie tego, o czym wspominasz), podczas gdy interfejs daje możliwość stworzenia wielu różnych implementacji (np. dla repozytoriów) - subtelna różnica, lecz nie należy jednego mylić z drugim. Nie będzie wielu różnych implementacji PenaltyService, ponieważ oczekiwane od aplikacji zachowanie jest jedno, zatem nie ma potrzeby tworzyć takiego interfejsu. - Patryk27 2019-01-10 12:37
Publiczne metody klasy to... jej interfejs :) - jarekr000000 2019-01-10 13:00

Pozostało 580 znaków

2019-01-09 00:22
0
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ąć?

edytowany 1x, ostatnio: must, 2019-01-09 00:23

Pozostało 580 znaków

2019-01-10 12:36
1
  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.

edytowany 1x, ostatnio: p_maciek, 2019-01-10 12:37
jak w takim razie zmieniać pola jak nie za pomocą setterów? Dlaczego settery są złe? - must 2019-01-10 17:10

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