• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Rzuci ktoś okiem na kod początkującego programisty?

Object Storage Arubacloud
0 głosów
458 wizyt
pytanie zadane 23 października 2018 w Java przez niezalogowany
edycja 7 listopada 2018

Cześć,

od początku października uczę się samodzielnie Javy. Napisałem konsolowy program symulujący konto bankowe. Mógłby ktoś rzucić okiem i dać opinię jak wygląda mój kod?

Poniżej link:

Kod na githubie

PS. Powyższy link już nie działa. Zaktualizowany kod uwzględniający uwagi w linku poniżej:

Kod po modyfikacji

1
komentarz 23 października 2018 przez adrian17 Ekspert (346,320 p.)
Słabo znam Javę, więc tylko drobny feedback: wrzuciłeś do repo pliki, które nie powinny trafiać do repo (na przykład .class). Poczytaj o plikach .gitignore :)
komentarz 23 października 2018 przez niezalogowany
Poczytam, dzięki
komentarz 24 października 2018 przez miro Pasjonat (23,870 p.)
Otworzyłem jeden plik i zrezygnowałem z analizy po zobaczeniu switchu. Unikaj zagnieżdżonych ifów. Poczytaj jak możesz pozbyć się takich konstrukcji:
https://refactoring.guru/smells/switch-statements

4 odpowiedzi

+3 głosów
odpowiedź 24 października 2018 przez Aisekai Nałogowiec (42,190 p.)
edycja 24 października 2018 przez Aisekai

Jak na tak krótki czas, nie jest źle ale można coś poprawić. Więc:

1, Polskie nazewnictwo - pozbądź się go. Używaj angielskiego.

2. Komentarze - komentarze, które opisują po co jest dana funkcja/zmienna itd (np. BankAccount 20 linia) pozbądź się ich. Jeżeli uważasz, że coś takiego potrzebuje komentarza to albo masz złe nazewnictwo, albo funkcja coś za dużo robi. Komentarze zostawiaj tylko po to, aby wyjaśnić coś co może nie być aż tak oczywiste (np dlaczego w sprawdzaniu czy liczba jest pierwszą sprawdzasz do pierwiastka tej liczby) albo np opisując jakiegoś regexa.

3. W mainie u Ciebie za dużo się dzieje. W zupełności powinno wystarczyć coś jak: new Bank().run();.

4. Za dużo masz zagnieżdżeń (np BankAccount 57 linia). Pozbądź się tego np. rozbijając kod na funkcje/dodatkowe klasy.

5. Niepotrzebnie dwa razy otwierasz plik z Lokatą. Możesz przekazać plik/ścieżkę jednokrotnie. Np jeżeli plik istnieje i zostanie otwarty, przekaż ten plik jako parametr.

6. Nazywaj zmienne tak, żeby jasno mówiły do czego służą (nawiązanie do punktu 2.) np:

czyWykonanoZdarzenie = Saldo.getSaldo()
//(...)
historia[Historia.getLicznikZdarzen()].zapiszZdarzenieWyplaty(czyWykonanoZdarzenie - Saldo.getSaldo());

Do czego w końcu służy zmienna czyWykonanoZdarzenie? Nazwa sugeruje, że jest to jakaś flaga (zmienna typu bool) natomiast potem, używasz jej jako jakiejś liczby, odejmując coś od niej. Tak samo saldoLokaty. 

 if (saldoLokaty == 0){
                System.out.println("Nie masz lokaty takim numerze!"
)

Czy saldoLokaty określa wartość lokaty, czy numer lokaty?

7.  0 < nrLok && nrLok <= lokata.length && nrLok <= Lokata.getLiczbaLokat() <-- Po co tak długi, nic nie mówiący warunek? Nie prościej wyrzucić to do jakiejś funkcji i nazwać funkcje jakoś ładniej, tak żeby od razu mówiły o co chodzi? Nie jest to z twojego projektu, ale żeby pokazać o co chodzi.Lepiej zmienić taki zapis:

if(zarobki>0 && zarobki<85000)

Na np:

if(czyJestWPierwszymProguPodatkowym())

I warunek wyrzucić do funkcji czyChodziDoPrzedszkola().

8. if (myLok.getRodzajZdarzenia() != "Empty")  <-- Oj nie. Operator != nie służy do porównywania obiektów, jakimi niewątpliwie są Stringi.. Metoda equals do tego służy. Jeżeli już porównujesz zmienną String z tekstem, to lepiej zastosować Yoda expressions i zmienić to na: "Empty".equals(myLok.getRodzajZdarzenia()).

9. Odnośnie 8 punktu, zwykły String jest nieelegancki. Lepszym sposobem, jest używanie Enum'ów. 

10. Puste funkcje - np. zapiszStan(), nic nie wnoszą do projektu.

11. Masz zmienne prywatne i wygenerowane tylko gettery. Do zmiany stanów obiektów służą Ci bardziej biznesowe metody [niż settery]. Za to plus.

12. Używaj mniej zmiennych static. Mocno ogranicza to rozbudowę aplikacji. Lepiej żeby obiekt składał się z innego obiektu, w którym będziesz miał jako pola (nie statyczne) saldo i utworzył taki obiekt niż używał static. Zastanowiłbym się też nad tym, czy nie dodać jakiejś klasy User który by miał Listę lokat jako pole i w klasie User byłaby informacja na temat salda wszystkich lokat. 

Dodatkowo zmienne static utrudniają testowanie programów i mogą powodować niechciane zachowania. Globalny dostęp do zmiennej oznacza, że w każdym miejscu w programie, możesz zmienić jej wartość, co może utrudnić lokalizację błędu.

13. Funkcje takie jak np. otworzLokate() (w Lokata) mocno utrudniają możliwość przeniesienia tego na inne platformy. Jeżeli kiedyś byś chciał wykorzystać ją w programie z GUI (nie konsola)  pobieranie wartości za pomocą Skanera i konsoli może być bardzo ciężkie.

14.Funkcje wypłata() lepiej nazwać wypłać(). Funkcje wpłata() - wpłać().

15. Tak jak kolega napisał. Dławienie wyjątków to nie jest dobry pomysł. Tak samo jak nadużywanie ich oraz try-catch - https://en.wikipedia.org/wiki/Coding_by_exception .

To na tyle. 

komentarz 24 października 2018 przez niezalogowany
Bardzo dziękuję za ten obszerny i bardzo motywujący do dalszej pracy komentarz.

Już zabieram się za zgłębianie tych wszystkich informacji i ich zastosowanie.

Wielkie dzięki!
komentarz 24 października 2018 przez Mateusz51 Nałogowiec (28,180 p.)

@Aisekai, bardzo ładny komentarz:) łapka w górę 

+2 głosów
odpowiedź 7 listopada 2018 przez RafalS VIP (122,820 p.)
edycja 7 listopada 2018 przez RafalS

Review nowego kodu:

  1. Nie dodawaj plików konfiguracyjnych (nbproject) IDE do repo.
  2. new BigDecimal("0"); czemu nie konstruktor z intem? Po co string?
  3. Troche źle to wszystko projektujesz. Za dużo klas utilsowych - z samymi metodami statycznymi bez stanu.
  4. Nazwy klas sa kiepskie. Widze klase Message. Mysle ze pewnie bedzie reprezetowała jakąs wiadomośc. Jaką? Nie wiem. Nie sprecyzowaleś. Otwieram a w srodku jakis worek na metody statyczne do printowania wiadomosci na konsole i co wiecej żadna metoda nie ma czasownika w nazwie, bo właściwie nic nie robi. Ta klasa nie powinna istnieć, to powinna być jakaś mapa String, String wczytywana z zewnątrz z jakiegoś pliku konfiguracyjnego np jsona.
    Dodatkowo nazwa klasy nie mowi nic o wypisywaniu na ekran. Więc zamykasz się na modyfikacje wyjscia na inne.
  5. Klasa AccountBalance jest nierozłanczalnie spięta z klasą Message. Wyobraź sobie, że kiedyś zrezygnujesz z wypisywania wszystkiego na ekran tylko będziesz chciał się od tej klasy dowiedziec czy udalo sie zmniejszych stan konta. Niestety sie nie dowiesz bo ta metoda wypisuje na ekran a nie zwraca wyniku. Wypisanie na ekran powinno dziac sie w klasie korzystajacej z AccountBalance, na wyższym poziomie abstrakcji. Osoba korzystajaca z tej klasy powinna decydowac co jak poinformuje o braku srodkow.
  6. Ponownie klasa AccountHistory błędnie deklaruje czym jest, bo jest tylko jednym wpisem w histori, jest informacją o pojedynczej transakcji a nie historią konta. Konstruktor na podstawie uzytkownika? Mało to logiczne. Dodatkowo korzystasz z tego usera w wielu metodach a za każdym razem przekazujesz go jako parametr. To jest programowanie proceduralne a nie obiektowe.
    Blad projektowy prowadzi do dziwnej sytuacji w klasie User, która posiada liste historii. Mało intuicyjne.
    user.history.get(user.history.size()-1)

    Co jesli size bedzie 0. Podpowiem - poleci wyjatek.
    Generalnie klasa strasznie nieczytelna, patrze na te nazwy metod i nie wiem po co ona jest i jak sie z niej korzysta.

  7. return user.history.get(user.history.size()-1).currentAccountBalanceAfterOperation.subtract(user.userCurrentAccountBalance.getCurrentAccountBalance());

    Poczytaj o prawie demeter, bo takie konstrukcje strasznie zamykają Twój kod na modyfikacje. Jesli zrobisz tak:

    obiekt.getInnyObiekt().getJeszczeInny().get().get().zrobCos()

    to w tym momencie zablokowales sobie szanse na modyfikacje kazdej z tych klas posrednich, bo wszystkie musza posiadac metody ktore tutaj wywolujesz. Nagle ktoś zmienia cos w tej przedostatniej klasie i Twoj kod, który jest logicznie bardzo daleko od tej klasy sie psuje. Nie daj Boże po cichu - wtedy nigdy nie znajdziesz tego buga.

  8. Nazwa klast DataFiles jest tak ogólna ze nie niestety nie wprowadza nic ;/. 
    Konstruktor i same metody statyczne. Dziwny design.

  9. Nie rozumiem podwójnie zagnieżdzonych try? Po co. Nie tak działa try-catch with resource. I po co wolac close() jesli od tego jest wspomniana konstrukcja. Jak cos sie nie uda w DataFiles to nikt poza loggerem o tym nie wie i program leci dalej jakby nigdy nic.

  10. Zero walidacji czegokolwiek.

  11. Deposit.getAnIndexOfEmptyDeposit. Po co taka dziwaczna petla while jesli od iterowania po tablicach sa petle for a jeszcze lepiej - streamy. W przebieg tej petli trzeb sie zaglebic zeby ogarnac co tam sie dzieje, w przeciwienstwie do gotowych metod - np streamow.

  12. ReportPrinter. Programowanie proceduralne. Statyczne metody przyjmujace dane.

  13. Odpowiedzialnoscia klasy transfer jest wczytanie kwoty? Troche to dziwne,

  14. Bardzo brzydki switch case zagniezdzony w innym switch casie ;/. Poczytaj o wzorcu strategii.

  15.     ArrayList<AccountHistory> history = new ArrayList<>();
    

    Interfejsy!!!
     

        List<AccountHistory> history = new ArrayList<>();
    

Krótkim słowem podsumowania - nie rozdzieliłeś warstw abstrakcji. Wypisywanie na ekran to inna odpowiedzialnosc niz liczenie stanu konta. One powinne byc rozdzielone.

Programujesz proceduralnie ;/

Programowanie obiektowe to polaczenie stanu (zmiennych) z zachowaniem (metodami). U Ciebie zachowanie jest odseparowane od stanu - metody statyczne przyjmujace dane. 

Brak interfejsów.

1
komentarz 7 listopada 2018 przez mbabane Szeryf (79,280 p.)

new BigDecimal("0"); czemu nie konstruktor z intem? Po co string? 

https://www.youtube.com/watch?v=1USy_dRBmsc 

komentarz 7 listopada 2018 przez RafalS VIP (122,820 p.)
Pierwszy raz widzę tą klase, wydawało się dziwne, ale jednak miało sens :P
0 głosów
odpowiedź 23 października 2018 przez Mateusz51 Nałogowiec (28,180 p.)
Unikaj static. Java to język obiektowy, korzystanie z static aby trzymać stan aplikacji nie jest dobrą metodą, szybko pogubisz się co i jak.
To jest taka główna uwaga.

Jeszcze mniejsze:

BankAccount co stoi na przeszkodzie aby porozdzielać każdy case na oddzielną metodę czy może nawet klasę?

Staraj się pisać w języku angielskim.

W klasie Menu dławisz wyjątki. Jest to zła praktyka, jak już to powinieneś coś wypisać.
komentarz 23 października 2018 przez niezalogowany
Dzięki za opinię.

Jak mógłbym zamienić static by dalej móc przechowywać jakieś liczniki dla klas?

BankAccount rzeczywiście rozbiję na mniejsze części bo sam już się zaczynam gubić.

No i wszystko na english przepiszę.
komentarz 23 października 2018 przez Wiciorny Ekspert (272,510 p.)

@Mateusz51,

Unikaj static.

rozumiem, że to stwierdzenie jak później dopisujesz tyczy się tylko kwesti " static' jako  

 aby trzymać stan aplikacji  

bo w innym wypadku, to całkowicie się nie zgodzę. I static jest bardzo potrzebny właśnie dobrze wykorzystany :) , w kwestii stanu "obiektu" obiektu to fakt, duże ryzyko zguby.  

komentarz 24 października 2018 przez Mateusz51 Nałogowiec (28,180 p.)

@niezalogowany, ważne o co Ci chodzi jako licznik? Jeśli jakieś id to static nie jest najgorszym rozwiązaniem.

Ale Tak jak Aisekai napisał static utrudnia rozbudowę aplikacji. Jak byś teraz chciał dodać kilku klientów z oddzielnym saldem musiał byś się nagimnastykowac. 

Jeśli potrzebujesz aby klasa coś pamiętała to możesz to robić w atrybutach obiektu. Wtedy mozesz inicjowac nowy obiekt za kazdyn razem gdy Ci potrzebny( np nowe saldo )oraz przechowywać  tak długo jak Ci będzie potrzebny. 

Potem zaczniesz podpinac bazy danych do projektów i trochę nakierunkuje Ci się sposób przechowywania danych.

Tak na koniec kiedy warto uzywac static. Zasada kciuka jest taka gdy funkcjonalnosc jest bezkontekstowa i bezstanowa. Przykład Math.abs za każdym razem dziala tak samo I wynik zależy tylko I wyłącznie od podanego wejścia funkcji. 

0 głosów
odpowiedź 7 listopada 2018 przez niezalogowany

@Aisekai, @Mateusz51

Trochę czasu mi to zajęło, ale posiedziałem "kilka" wieczorów nad moim kodem i teraz wygląda tak:

BankAccount_2 - GitHub.

Jeśli ktoś miałby chwilę by podzielić się komentarzem i wskazać co zmienić / poprawić będę bardzo wdzięczny.

Pamiętajcie, że jestem dopiero na początku mojej przygody z programowaniem ;)

PS. Nie działa jeszcze zapis do pliku historii operacji - sprawia mi to trochę trudność ale pracuję nad tym (przez to pierwsza operacja w historii ma nieco inną wartość niż mieć powinna).

Podobne pytania

0 głosów
0 odpowiedzi 195 wizyt
pytanie zadane 30 stycznia w PHP przez whiteman808 Obywatel (1,950 p.)
0 głosów
2 odpowiedzi 377 wizyt
–1 głos
1 odpowiedź 156 wizyt
pytanie zadane 8 stycznia 2020 w C i C++ przez Nabuchadonozor Gaduła (3,120 p.)

92,702 zapytań

141,615 odpowiedzi

320,180 komentarzy

62,060 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...