Ocena kodu - Aplikacja konsolowa Bank

0

Cześć,

Pracuję nad aplikacją konsolową banku (która już wcześniej tutaj wrzucałem, ale w bardzo początkowej fazie) i byłbym wdzięczny za pomoc w sprawdzeniu mojego kodu. To mój pierwszy większy projekt (oczywiście jak na moje możliwości), w którym ćwiczyłem programowanie obiektowe i przy okazji dokumentowanie kodu, więc zależy mi na opinii osób, które się na tym bardziej znają. Dodam jeszcze, ze nie umieszczałem klas w pakietach, bo jeszcze nie do końca wiem, jak to podzielić. Co można by poprawić w moim kodzie? Czy dokumentacja jest wystarczająca i dobrze napisana? To mój pierwszy raz, kiedy pisałem dokumentację, więc każda uwaga będzie na wagę złota.

Link do repozytorium: https://github.com/MaciekSm19/Bank

Z góry dziękuję za pomoc!

1

Na pierwszy rzut oka - fajnie. Opędzlujmy low hanging fruit:

    /**
     * <pre> Stores account's balance </pre>
     */
    private double balance = 0;

czy ten komentarz coś wnosi? :) Nie musisz komentować wszystkich pól ani metod, zwłaszcza jeśli są dobrze nazwane. Piszesz że "ćwiczysz dokumentowanie kodu" ale tutaj raczej nie o to chodzi.


        System.out.print("Ile pieniedzy chcesz wyplacic? ");

Hardkodzenie polskich stringów w kodzie średnio wygląda. Nie wyślesz tego projektu na zagraniczną rekrutację.

            else if(amount <= balance) {

Używaj autoformattera, skoro używasz IJ.

     * <pre>Allows user balance money to the account.</pre>

Coś to po angielskawu jest chyba :)

  public class Address {

IntellIJ mówi mi że używasz Javy 22, więc masz już rekordy - użyj ich.

Ale najważniejsze:

Czy dokumentacja jest wystarczająca i dobrze napisana

Nie masz ani jednego testu. A testy są (a raczej: mogą być) bardzo dobrą dokumentacją.

2
  1. Praktycznie każdy komentarz w Twojej aplikacji jest niepotrzebny, wywal je.
  2. To że aplikacja printuje polski tekst, na początku jest spoko, do tego bym się nie przyczepiał.
  3. Niepotrzebne settery i gettery. Ja bym zrobił po prostu pola publiczne.
  4. Nazwa UserContactData jest dziwna. Czemu nie UserContact? tak samo UserLoginData -> Credentials. I wywal niepotrzebne setter i gettery, one nic nie dają.
0

Ok będę właśnie teraz wiedział, żeby nie komentować takich rzeczy

Na pierwszy rzut oka - fajnie. Opędzlujmy low hanging fruit:

    /**
     * <pre> Stores account's balance </pre>
     */
    private double balance = 0;

czy ten komentarz coś wnosi? :) Nie musisz komentować wszystkich pól ani metod, zwłaszcza jeśli są dobrze nazwane. Piszesz że "ćwiczysz dokumentowanie kodu" ale tutaj raczej nie o to chodzi.

Czyli stosować normalnie polskie znaki, czy pisać całkowicie po angielsku?


        System.out.print("Ile pieniedzy chcesz wyplacic? ");

Hardkodzenie polskich stringów w kodzie średnio wygląda. Nie wyślesz tego projektu na zagraniczną rekrutację.

Ok

            else if(amount <= balance) {

Używaj autoformattera, skoro używasz IJ.

Ten błąd akurat wynika z tego, ze zmieniłem nazwę zmiennej i automatycznie zmieniło mi wszystkie użycia 🤦

     * <pre>Allows user balance money to the account.</pre>

Coś to po angielskawu jest chyba :)

Mój kurs jest trochę przestarzały, więc dlatego nie słyszałem jeszcze o rekordach, ale zaraz doczytam

  public class Address {

IntellIJ mówi mi że używasz Javy 22, więc masz już rekordy - użyj ich.

Co do testów to właśnie będę sie brał zaraz za ogarnięcie tego

Ale najważniejsze:

Czy dokumentacja jest wystarczająca i dobrze napisana

Nie masz ani jednego testu. A testy są (a raczej: mogą być) bardzo dobrą dokumentacją.

Dzięki za ocenę kodu!

3
Maciek_SK8 napisał(a):

Co do testów to właśnie będę sie brał zaraz za ogarnięcie tego

Do testów podejdź ostronżnie - z testami jest taki problem, że jest bardzo dużo niskiej wartości tutoriali na internecie. Na prawdę sensownych tutoriali jest bardzo mało, także trzeba znaleźć odpowiednie źródło 😕

Tutaj jest jedno dobre, jeśli chcesz zacząć pisać testy, zacznij od tego:

@Maciek_SK8 Co do designu aplikacji, myślę że Twoim największym błędem jest zmienna statyczna users w UserOperations. To jest bardzo słabe.

0
Riddle napisał(a):
  1. Praktycznie każdy komentarz w Twojej aplikacji jest niepotrzebny, wywal je.
  2. To że aplikacja printuje polski tekst, na początku jest spoko, do tego bym się nie przyczepiał.
  3. Niepotrzebne settery i gettery. Ja bym zrobił po prostu pola publiczne.
  4. Nazwa UserContactData jest dziwna. Czemu nie UserContact? tak samo UserLoginData -> Credentials. I wywal niepotrzebne setter i gettery, one nic nie dają.

  1. Zaraz pousuwam, ale nie lepiej będzie zostawić te dłuższe, które bardziej opisują metody?

  2. Cały czas myślałem, ze lepiej robić zmienne prywatne i operować nimi poprzez gettery i settery. W jakim przypadku wtedy ich używać

  3. Już idę to pozmieniać

Dzięki za ocenę kodu!

0
Maciek_SK8 napisał(a):
  1. Zaraz pousuwam, ale nie lepiej będzie zostawić te dłuższe, które bardziej opisują metody?

Nie. Wszystkie Twoje komentarze są niepotrzebne.

Jeśli uważasz że niektóre Twoje metody są nieczytelne i wymagają wyjaśnienia, to odpowiednim wyjściem jest zrefaktorować te metody, i poprawić je żeby były bardziej czytelne. Najlepszym sposobem jest dobranie odpowiednich nazw zmiennych i metod.

Maciek_SK8 napisał(a):
  1. Cały czas myślałem, ze lepiej robić zmienne prywatne i operować nimi poprzez gettery i settery. W jakim przypadku wtedy ich używać

Zmienne prywatne są lepsze niż publiczne - im mniej rzeczy publicznych, tym łatwiej będzie zmienić taką klasę.

Tylko że jeśli masz prywatne pole, i dodajesz do niej setter/getter, to ta zmienna praktycznie staje się publiczna (bo i tak jest do niej dostęp z zewnątrz, za pomocą tych setterów). Więc lepiej jest sobie uprościć życie, i zrobić żeby pola były public.

Twoja intuicja że prywatne pola są lepsze jest trafna - tylko żeby pole było prywatne, to nie może mieć setterów i getterów.

Ja na Twoim miejscu skupiłbym się na usunięciu zmiennej statycznej users z UserOperations. To jest największa wada projektowa w Twojej aplikacji. W ogóle sugerowałbym Ci nie korzystanie ze zmiennych statycznych na tym etapie.

0
Riddle napisał(a):
Maciek_SK8 napisał(a):

Co do testów to właśnie będę sie brał zaraz za ogarnięcie tego

Do testów podejdź ostronżnie - z testami jest taki problem, że jest bardzo dużo niskiej wartości tutoriali na internecie. Na prawdę sensownych tutoriali jest bardzo mało, także trzeba znaleźć odpowiednie źródło 😕

Tutaj jest jedno dobre, jeśli chcesz zacząć pisać testy, zacznij od tego:

Dzięki za podpowiedz z tymi testami 😀

@Maciek_SK8 Co do designu aplikacji, myślę że Twoim największym błędem jest zmienna statyczna users w UserOperations. To jest bardzo słabe.

Zmienię na zwykła zmienna

0
Riddle napisał(a):
Maciek_SK8 napisał(a):
  1. Zaraz pousuwam, ale nie lepiej będzie zostawić te dłuższe, które bardziej opisują metody?

Nie. Wszystkie Twoje komentarze są niepotrzebne.
Jeśli uważasz że niektóre Twoje metody są nieczytelne i wymagają wyjaśnienia, to odpowiednim wyjściem jest zrefaktorować te metody, i poprawić je żeby były bardziej czytelne. Najlepszym sposobem jest dobranie odpowiednich nazw zmiennych i metod.

W takim przypadku po co są te komentarze? Czy po prostu nie używać ich na tym etapie nauki?

Maciek_SK8 napisał(a):
  1. Cały czas myślałem, ze lepiej robić zmienne prywatne i operować nimi poprzez gettery i settery. W jakim przypadku wtedy ich używać

Zmienne prywatne są lepsze niż publiczne - im mniej rzeczy publicznych, tym łatwiej będzie zmienić taką klasę.

Tylko że jeśli masz prywatne pole, i dodajesz do niej setter/getter, to ta zmienna praktycznie staje się publiczna (bo i tak jest do niej dostęp z zewnątrz, za pomocą tych setterów). Więc lepiej jest sobie uprościć życie, i zrobić żeby pola były public.

To kiedy w ogóle je stosować? Jeśli nie do odczytywania i modyfikacji pół prywatnych?

Twoja intuicja że prywatne pola są lepsze jest trafna - tylko żeby pole było prywatne, to nie może mieć setterów i getterów.

Przynajmniej z tym dobrze myślę 😀.

Ja na Twoim miejscu skupiłbym się na usunięciu zmiennej statycznej users z UserOperations. To jest największa wada projektowa w Twojej aplikacji. W ogóle sugerowałbym Ci nie korzystanie ze zmiennych statycznych na tym etapie.

Tak, jak już pisałem wcześniej zaraz to naprawie.

2 
private double balance = 0;

polecam klasygg i w testach porobić sobie operacje dodawania i odejmowania ułamków na doublach 😄

1

W takim przypadku po co są te komentarze? Czy po prostu nie używać ich na tym etapie nauki?

Komentarze są fragmentem kodu ignorowanym przez kompilator, ewentualnie są przetwarzane przez narzędzia generujące spisy klas i metod. Poza tym, IDE takie jak IJ potrafi taki komentarz ładnie sformatować i wyświetlić w popupie. Możesz i Pana Tadeusza tam wpisać i będzie ok.

W przykładzie który wskazałem (pewnie jest więcej, ale to było pierwsze z góry) masz komentarz Stores account's balance - ale my już wiemy, że pole nazywa się balance i należy do klasy Account. Nie ma sensu pisać tego samego 2 razy, już w kodzie wyraziłeś to co chciałeś.

1
Maciek_SK8 napisał(a):

W takim przypadku po co są te komentarze? Czy po prostu nie używać ich na tym etapie nauki?

Takie komentarze jak napisałeś są niepotrzebnie zupełnie.

Natomiast sama funkcja komentarzy w kodzie źródłowym jest potrzebna, żeby na chwilę zakomentować kawałek kodu, żeby napisać jakieś istotne informacje których nie da się wyrazić kodem, etc.

Znajdź książkę "Clean Code" autorstwa Roberta Martina, znajdź w niej rozdział 4, "Komentarze". Warto przeczytać.

Tutaj masz PDF: Czysty-kod.-Podręcznik-dobrego-programisty.zip

Maciek_SK8 napisał(a):

To kiedy w ogóle je stosować? Jeśli nie do odczytywania i modyfikacji pół prywatnych?

Moim zdaniem, najlepiej nigdy. Settery i gettery to słaby design. Jest na prawdę bardzo mało przypadków w których setter albo getter ma sens. Jak masz ochotę użyć settera/gettera, to to jest dobry znak że tak na prawdę chcesz zrobić pole publiczne.

1 
    void depositMoney(User user) {
        System.out.print("Ile pieniedzy chcesz wplacic? ");
        double amount = scanner.nextDouble();
        if (checkIfUserExists(user.getUserLoginData().getLogin())) {
            balance += amount;
            System.out.println("Wplata zatwierdzona! Obecny stan konta wynosi: " + balance);
        } else
            System.out.println("Nie mozna wykonac tej operacji.");
    }

używasz usera i na tej podstawie zmieniasz stan konta? 😉

wydziel sobie osobną metodę do depozytu, coś ala:

double depositMoneyAndGetBalance(double amount) {
  return balance + amount;
}

też użyłem double, choć najlepiej użyć BigDecimal. Przynajmniej to można przetestować 😀

1
Riddle napisał(a):

Moim zdaniem, najlepiej nigdy. Settery i gettery to słaby design. Jest na prawdę bardzo mało przypadków w których setter albo getter ma sens. Jak masz ochotę użyć settera/gettera, to to jest dobry znak że tak na prawdę chcesz zrobić pole publiczne.

Taka dygresja, nie chciałem zaciemniać tematu, ale tak wyszło. Co do setterów - zgoda. Ale serio nie rozumiem, czemu polecasz używanie pól publicznych w Javie? Jaki to ma sens?
Po to właśnie używa się pól prywatnych, konstruktorów i getterów (lub po prostu recordów), że po utworzeniu obiektu nic nie zmieni jego stanu. No chyba, że refleksją, ale to inna para kaloszy.
A w przypadku jak masz pola publiczne to każdy inny wątek może zmienić stan, co jest ogólnie bardzo złym rozwiązaniem.

1

Zaraz pousuwam, ale nie lepiej będzie zostawić te dłuższe, które bardziej opisują metody?

Kometarze tak jak Ty je piszesz, stawiaja wszystkie osoby świeżo uczące sie programować, przed przeczytanie lub dostaniem pierwszej zjebki. Dlaczego jest stawiają? Bo piszą tak jak kod jest napisany w dokumentacji, lub kursie, a że rolą dokumentacji jest wytłumaczenie wszystkiego, to wyszstko komentuja. W każdym innym przypadku sie tak nie robi.

Wiec nie stawia sie tak kometarzy w ogóle. Kometarze są dobre i warto je dawać, gdy masz nie oczywiste zależności, gdy cos hakujesz, naprawiałeś buga, gdy masz dług techniczny i TO DO. Ogólnie gdy jezyk programowania nie pozwala przekazać istotnej informacji o programie która innaczej zaginie.

1

Masz błąd w tej linijce:

System.out.println("\nWitaj w kreatorze uzytkownika. Musisz podac niezbedne dane, aby zarejestrowac sie w aplikacji baku.\n");
1
trojanus__ napisał(a):
Riddle napisał(a):

Moim zdaniem, najlepiej nigdy. Settery i gettery to słaby design. Jest na prawdę bardzo mało przypadków w których setter albo getter ma sens. Jak masz ochotę użyć settera/gettera, to to jest dobry znak że tak na prawdę chcesz zrobić pole publiczne.

Taka dygresja, nie chciałem zaciemniać tematu, ale tak wyszło. Co do setterów - zgoda. Ale serio nie rozumiem, czemu polecasz używanie pól publicznych w Javie? Jaki to ma sens?
Po to właśnie używa się pól prywatnych, konstruktorów i getterów (lub po prostu recordów), że po utworzeniu obiektu nic nie zmieni jego stanu. No chyba, że refleksją, ale to inna para kaloszy.
A w przypadku jak masz pola publiczne to każdy inny wątek może zmienić stan, co jest ogólnie bardzo złym rozwiązaniem.

Nie polecałem używania pól publicznych. Jeśli możesz zrobić pole prywatne bez getterów, to jest to sytuacja idealna. Czasem trzeba dodać funkcje które zwraca pole na zewnątrz, i to też jest w porządku (chociaż widziałem dużo getterów które łamią enkapsulację w swoim czasie). Takie pole powinno zostać prywatne.

Ale jeśli czujesz że chcesz zmodyfikować pole z zewnątrz, i dodajesz setter - to moim zdaniem to jest błąd. Skoro i tak chcesz modyfikować pole z zewnątrz, to po prostu zrób je public. Jeśli nie chcesz modyfikować pola, to usuń setter i zostaw pole private.

Czyli w skrócie:

  • najlepsze jest pole private. Jeśli możesz mieć pole bez getterów i setterów - zrób to! To jest najlepsze.
  • jeśli trzeba je zwrócić, to drugim najlepszym wyjściem jest private + getter,
  • jeśli chcesz mieć stan w obiekcie to zmień property za pomocą odpowiednich funkcji;
  • a jeśli chcesz żeby pole było modyfikowalne z zewnątrz, zrób je public.

Nie widzę sensu żeby dodawać setter. Jak masz kod, taki jaki zaprezentowałes, który ma private+getter+setter, to praktycznie masz pole public tylko brzydziej i trudniej napisane.

0

Dzięki. Wezmę się za czytanie tej książki

Znajdź książkę "Clean Code" autorstwa Roberta Martina, znajdź w niej rozdział 4, "Komentarze". Warto przeczytać.

Tutaj masz PDF: Czysty-kod.-Podręcznik-dobrego-programisty.zip

1
Maciek_SK8 napisał(a):

Czyli stosować normalnie polskie znaki, czy pisać całkowicie po angielsku?

Generalnie chcesz dazyc do tego zeby nie trzymac zadnych tekstow w kodzie. Wszystkie teskty dobrze trzymac w pliku konfiguracyjnym z podzialem na rozne jezyki. Aplikacja wczytuje sobie dane tlumaczenie na podstawie wybranego przez uzytkownika jezyka.

0
Tgc napisał(a):
Maciek_SK8 napisał(a):

Czyli stosować normalnie polskie znaki, czy pisać całkowicie po angielsku?

Generalnie chcesz dazyc do tego zeby nie trzymac zadnych tekstow w kodzie. Wszystkie teskty dobrze trzymac w pliku konfiguracyjnym z podzialem na rozne jezyki. Aplikacja wczytuje sobie dane tlumaczenie na podstawie wybranego przez uzytkownika jezyka.

Aaa teraz juz rozumiem o co chodzi. Dzięki!

0

Mam jeszcze pytanie dotyczące podziału klas na pakiety. Na tą chwilę wymyśliłem cos takiego (nie wiedziałem co zrobić z klasami Main i Menu):
screenshot-20240728091438.png

1

Poczytaj o:

  1. https://nullpointerexception.pl/architektura-warstwowa-sposob-na-organizacje-kodu/
  2. https://phauer.com/2020/package-by-feature/
  3. https://bulldogjob.pl/readme/architektura-heksagonalna-w-javie-w-3-minuty

I obejrzyj to (ale dopiero po przeczytaniu tesktów z poprzednich linków):

0
RequiredNickname napisał(a):

Poczytaj o:

  1. https://nullpointerexception.pl/architektura-warstwowa-sposob-na-organizacje-kodu/
  2. https://phauer.com/2020/package-by-feature/
  3. https://bulldogjob.pl/readme/architektura-heksagonalna-w-javie-w-3-minuty

I obejrzyj to (ale dopiero po przeczytaniu tesktów z poprzednich linków):

Wszystko przeczytałem i obejrzałem. Wykracza to trochę ponad mój poziom, ale wydaje mi się, ze najważniejsze rzeczy zrozumiałem. W taki sposób to rozpisałem:

Podział warstwowy klas:

  1. Warstwa danych:
    • User,
    • User Contact,
    • User Personal Data
    • Credentials,
    • Address.
  2. Warstwa logiki biznesowej:
    • User Operations,
    • Account,
    • Account Mem.
  3. Warstwa prezentacji:
    • Menu,
    • Main.

Podział funkcjonalny klas:

  1. Zarządzanie użytkownikiem:
    • User,
    • User Contact,
    • User Personal Data,
    • Credentials,
    • Address,
    • User Operations.
  2. Zarządzanie kontem:
    • Account,
    • Account Menu.
  3. Zarządzanie aplikacją:
    • Menu,
    • Main.

Dobrze to rozumiem?

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.

Utwórz konto