Ocena wypożyczalni samochodów

0

Cześć. Przychodzę do Was z prośba o sprawdzenie wypożyczalni samochodów.
Jest to mój pierwszy projekt w którym użyłem mavena, testów jednostkowych, mockito czy slf4j.

Uprzedzam pytania, slf4j użyłem z tego względu, iż czytałem w necie, że lepiej jest tego użyć aniżeli sys.out.
Jeżeli chodzi o Client/Worker DataGetter, nie ma tam jakis ifów, jeżeli ktoś by wpisał litery zamiast cyfr w peselu. Wiadomo, program się wysypie, ale nie skupiałem się zbytnio nad tym, by pobieranie danych było idealne, a na ogólnym ćwiczeniu programowaina.

Chciałbym się takżę Was poradzić, co powinienem dalej zrobić? Jakiś nowy projekt, moze o czyms poczytać.

Dzięki wielkie za każdą odpowiedź.
Link do githuba: https://github.com/must1/CarRental

4
  1. https://github.com/must1/CarR[...]t/java/car/model/CarTest.java - ten test nie pokrywa absolutnie nic - równie dobrze mógłbyś tam wszędzie mieć assertEquals(1, 1); z reguły nie pisze się testów do modeli, ponieważ powinno się testować zachowanie (a modele zachowania na ogół nie mają, gdyż są tylko pojemnikiem na dane).

  2. https://github.com/must1/CarR[...]ava/car/model/ClientTest.java - jak wyżej.

  3. https://github.com/must1/CarR[...]ers/ClientDataGetterTest.java - ten test za to jest już jak najbardziej poprawny, ponieważ sprawdza zachowanie klasy ClientDataGetter (tzn. czy poprawnie wczytuje dane oraz czy poprawnie generuje obiekt) :-)

  4. when(carRentalStorageMock.getAllCustomers()).thenReturn(listOfClients); - czy na pewno musisz tutaj wykorzystywać mocki?

  5. https://github.com/must1/CarR[...]/DB/CarRentalSQLDatabase.java - god object, brak separation of concerns (metody zarówno operują na bazie, jak i interfejsie użytkownika - System.out.println).

  6. https://github.com/must1/CarR[...]rental/model/RentingACar.java - modele powinny być nazwane z wykorzystaniem rzeczownków; RentingACar nim nie jest. Może CarRental?

Od siebie mogę polecić poczytanie o wzorcu repository - sprawdzi się tutaj idealnie.


Chciałbym się takżę Was poradzić, co powinienem dalej zrobić?

Sam sobie odpowiedziałeś:

[...] nie skupiałem się zbytnio nad tym, by pobieranie danych było idealne, a na ogólnym ćwiczeniu programowaina.

Zaimplementuj tę brakującą walidację ;-)

0
Patryk27 napisał(a):
  1. https://github.com/must1/CarR[...]t/java/car/model/CarTest.java - ten test nie pokrywa absolutnie nic - równie dobrze mógłbyś tam wszędzie mieć assertEquals(1, 1); z reguły nie pisze się testów do modeli, ponieważ powinno się testować zachowanie (a modele zachowania na ogół nie mają, gdyż są tylko pojemnikiem na dane).

Będzie usunięte

  1. https://github.com/must1/CarR[...]ava/car/model/ClientTest.java - jak wyżej.

To też

  1. when(carRentalStorageMock.getAllCustomers()).thenReturn(listOfClients); - czy na pewno musisz tutaj wykorzystywać mocki?

Wydaje mi się, ze tak. Szczerze powiedziawszy nie widzę innego rozwiązania :D

  1. https://github.com/must1/CarR[...]/DB/CarRentalSQLDatabase.java - god object, brak separation of concerns (metody zarówno operują na bazie, jak i interfejsie użytkownika - System.out.println).

Tutaj sys.out zamienie na loggery, bo widzę ze zapomniałem ale nadal będzie się to pokrywało z interfejsem. Jest ich mało, bo może z 2-3 ale faktycznie są. Może jakaś obsługa błędów w innej klasie?

  1. https://github.com/must1/CarR[...]rental/model/RentingACar.java - modele powinny być nazwane z wykorzystaniem rzeczownków; RentingACar nim nie jest. Może CarRental?

Zamienię nazwę.

Od siebie mogę polecić poczytanie o wzorcu repository - sprawdzi się tutaj idealnie.

Poczytam na pewno!


Chciałbym się takżę Was poradzić, co powinienem dalej zrobić?

Sam sobie odpowiedziałeś:

[...] nie skupiałem się zbytnio nad tym, by pobieranie danych było idealne, a na ogólnym ćwiczeniu programowaina.

Zaimplementuj tę brakującą walidację ;-)

Tak, ale to jest chwila moment, dodanie kilku ifów ew. jakieś wyjątki. Ale co potem?

1

Tutaj sys.out zamienie na loggery, [...]

Nie, nie - całkowicie nie o to chodzi czy masz tam System.out.println, czy sys.out.

Masz klasę, która ma ogrom odpowiedzialności - poczytaj o wzorcu repozytorium i zrefaktoryzuj ją, pamiętając o tym, że interfejs użytkownika a dostęp do danych to dwie odrębne rzeczy, więc powinny być też rozdzielone między odrębne klasy.

Ale co potem?

Powoli - najpierw wprowadź poprawki, dodaj walidację i pogadamy ;-)

0

Update poleciał na repozytorium. Zrobiłem wszystko prócz tego repository pattern. Ciężko mi strasznie ogarnąć jak to dokładnie działa, jakies business layer etc... Poszukam na pewno jeszcze info o tym na innych stronach, ale na pewno dłużej zejdzie o ile to zrobie.

0

Szczerze powiedziawszy nie wiem co tu mogę zmienić jeszcze oprócz tego wypisywania jakiś informacji. Interfejs jest, metody implementujące ten interfejs są.
Jeżeli chodzi o to wypisywanie, to nie wiem czy zrobić jakąś klase tak jak piszesz. Coś na zasadzie tego:
Jeżeli mam metode taką:

@Override
    public void makeCarAvailable(Car car) throws SQLException {
       ...

        if (isAvailable) {
            preparedStatement = connection.prepareStatement("update car " + " set available='1'" + " where brand=? AND productionYear=?");
            preparedStatement.setString(1, car.getBrand());
            preparedStatement.setString(2, car.getProductionYear());
            preparedStatement.executeUpdate();
            System.out.println(car.getBrand() + " was made unavailable");
        } else {
            logger.info("No " + car.getBrand() + " in system!");
        }
    }

To czy to wyświetlanie zrobić na zasadzie:

metoda()
{
...
metoda_wyswietlajaca_komunikat();
}

A w tej metodzie własnie to: logger.info("No " + car.getBrand() + " in system!");

2

Twoja metoda nadal robi dwie rzeczy:

  1. Zmienia coś w bazie.
  2. Informuje użytkownika o tej zmianie.

Problem w tym podejściu jest taki, że wymusza ona pewne zachowanie, którego nie da się obejść [1] - gdybyś chciał np. dodać do swojej aplikacji GUI, nie mógłbyś tej metody wykorzystać.

A tak wcale być nie musi:

public boolean makeCarAvailable(Car car) throws SQLException {
    /* ... */

    if (!isAvailable) {
        return false;
    }

    preparedStatement = connection.prepareStatement("update car " + " set available='1'" + " where brand=? AND productionYear=?");
    preparedStatement.setString(1, car.getBrand());
    preparedStatement.setString(2, car.getProductionYear());
    preparedStatement.executeUpdate();

    return true;
}

/* ... gdzieś dalej w kodzie ... */

if (obj.makeCarAvailable(car)) {
    System.out.println("Udało się.");
} else {
    System.out.println("Nie udało się.");
}

Zauważ, że w tym konkretnym wypadku efekt (z punktu widzenia użytkownika) będzie identyczny, lecz w kodzie zaszła ogromna zmiana: część odpowiedzialna za bazę danych nie wie już nic o interfejsie użytkownika (nie ma żadnego System.out.println czy dowolnej innej formy komunikacji) - moglibyśmy zamiast TUI mieć GUI, i tej metodzie to zwisa ;-)


[1] fachowo chodzi tutaj o separation of concerns - dostęp do bazy i komunikacja z użytkownikiem to dwie niepowiązane wprost ze sobą czynności, stąd powinny być rozdzielone również w kodzie.

0

Dobra udało mi się to zrobić. Repozytorium zaktualizowane.

0

Co powinienem robić dalej i czy coś ewentualnie jeszcze poprawić?

3

Jest coraz ładniej! :-)

  1. https://github.com/must1/CarR[...]ntal/DB/CarRentalStorage.java - ten interfejs zarządza wieloma rodzajami obiektów (samochodami, klientami i wypożyczeniami), przez co ma bardzo szeroką odpowiedzialność. Wygodniej będzie wydzielić trzy odrębne interfejsy: CarStorageInterface, ClientStorageInterface oraz RentalStorageInterface. Klasy implementujące te interfejsy mogą być nazwane np. MySQLCarStorage, MySQLClientStorage itd.

  2. Zdecyduj się, czy chcesz wykorzystywać rzeczownik client czy customer ;-)

  3. Za co odpowiedzialne są klasy znajdujące się w tej - https://github.com/must1/CarR[...]in/java/car/rental/activities - przestrzeni?

  4. https://github.com/must1/CarR[...]tal/CarRentalEngine.java#L126 - troszeczkę długa ta linijka; dlaczego nie porozdzielasz tego na osobne wywołania logger.info?

  5. Dlaczego np. ta - https://github.com/must1/CarR[...]arRentalSQLDatabase.java#L138 - metoda ma w sobie System.out.println? Dostęp do bazy miał być całkowicie uniezależniony od interfejsu użytkownika.

  6. https://github.com/must1/CarR[...]arRentalSQLDatabase.java#L117 - ta metoda jest trochę długa. Co powiesz o takim podejściu?

  7. Dlaczego statement, preparedStatement oraz result są polami klasy, a nie zmiennymi lokalnymi?

  8. Wszędzie posługujesz się warunkiem WHERE brand=? AND productionYear=? - dlaczego nie utworzysz po prostu pola id?

  9. https://github.com/must1/CarR[...]arRentalSQLDatabase.java#L172 - listOfClients brzmi brzydko i długo; raczej pisze się po prostu clients - z racji wykorzystania rzeczownika w liczbie mnogiej wiadomo, że to lista czy tablica.

  10. https://github.com/must1/CarR[...]arRentalSQLDatabase.java#L195 - ta metoda jest nazwana getRentedCars, co sugeruje, że zwraca listę samochodów, co nie jest prawdą. Może getClientRentals?

  11. https://github.com/must1/CarR[...]tters/InputTakerTest.java#L35 - straaaaaaasznie długa nazwa; pomyśl nad czymś krótszym.

  12. Twoje testy w klasie InputTakerTest nie pokrywają przypadków skrajnych:
    a) co się stanie w momencie, gdy odpalę takeStringInput na pustym scannerze?
    b) co się stanie w momencie, gdy odpalę takeIntInput / takeLongInput z ujemną liczbą w buforze?
    c) co się stanie w momencie, gdy odpalę takeIntInput / takeLongInput z niepoprawną liczbą w buforze? (np. "asdf")

  13. Czym rózni się https://github.com/must1/CarR[...]a/getters/InputTakerTest.java od https://github.com/must1/CarR[...]/getters/InputTakersTest.java?

  14. Wszystko, oprócz ostatnich trzech metod w https://github.com/must1/CarR[...]tal/CarRentalOptionsTest.java jest zbędne, ponieważ tak właściwie nic one nie sprawdzają. Co takiego pokrywa Ci ten test? Jeśli odpalę metodę X, to uruchomi się metoda X - to nic nie znaczy. Lub - w inną stronę - podaj mi przypadek skrajny, który sprawi, że ten test się nie powiedzie. Jeśli taki nie istnieje, test jest zbędny (ponieważ nic nie pokrywa). Lub - jeszcze inaczej: jeśli jesteś w stanie napisać test w ogóle nie myśląc, tylko robiąc kopiuj-wklej i zmieniając nazwę metody, taki test jest zbędny ;-)

  15. https://github.com/must1/CarR[...]ataBaseCommunication.java#L14 - ta klasa wcale nie potrzebuje konkretnie CarRentalSQLDatabase - wystarczy CarRentalStorage. Dzięki temu mógłbyś podmienić implementację np. z bazy danych na pliki, i nic w tej klasie nie będzie wymagało zmiany - na tym polega cała zaleta wykorzystywania interfejsów.

  16. https://github.com/must1/CarR[...]/rental/CarRentalOptions.java - jaka jest w zasadzie odpowiedzialność tej klasy i po co ona istnieje?

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