Code Review. Kalkulator Model-View-Controller

Question

Siemanko,

Stworzyłem kalkulator stosując wzorzec architektoniczny Model-View-Controller oraz bibliotekę Swing. Chciałbym żebyście zrobili code review, z chęcią i pokorą przyjmę wszystkie rady. Proszę tylko bez wyzwisk. Wszelkie zastrzeżenia odnośnie zastosowania wzorca oraz innych aspektów. Kalkulator miał działać zgodnie z kolejnością wykonywania działań oczywiście, dbając o to by nie wprowadzono np. podwójnego znaku mnożenia. Docelowo ma rozwiązywać równania, ale póki co equation solver jeszcze nieokodowany. Z góry dzięki i nie krzyczcie hah ;)

Tutaj Github: https://github.com/TheRebMon/KalkulatorMVC .

 

Answer 1

Dobra, kod jest całkiem fajny. Bardzo dobrze, że piszesz po angielsku i ze masz odpowiednią strukturę plików. Ja generalnie nie znam się na swingu więc nie wypowiem się na temat użytych komponentów ze swinga ale skupię się na czytelności kodu. 

W całym kodzie brakuje jednolitego formatowania:

- formatowanie kodu (gdzie niegdzie za duzo nowych lini, jakiego IDE uzywasz? - kazdy ma jakis auto formater wbudowany)

klasa ButtonsPanel

- wywal komentarze (po to jest historia na gicie zeby sprawdzac poprzednie wersje)- twoje menu (linia 56+) wyrzuc do innej funkcji

- showUI() i to wszystko co sie dzieje w srodku mozna zrobic za pomoca petli i to bedzie lepeij skalowalne. 

klasa MainPanel

- linia 93 -> if (for(if - nie idzie sie w tym polapac i na pewno da sie to zrobic bez takiego zagniezdzenia. PAMIETAJ if w ifie to nie jest dobra praktyka, a ty tam jeszcze petle dajesz :D

klasa Model

- to samo co wyzej odnosnie for(if(if))) w az 4 miejscach

klasa Wrapper

- to samo co wyzej for(if(if(switch)))) - koniecznie to popraw :)

 

Comments

Bardzo dziękuję za komentarz. Daje mi to świeże spojrzenie na kod :) Szczególne jeśli chodzi o te ify, które dla mnie wydawały się dość naturalne, okazały się być nienajlepsze. Poprawię i za jakiś czas zapraszam zerknąć znowu  

---

Spoko a jakiego IDE uzywasz?

---

Eclipse Java

Używam auto-formatowania, ale ja stawiam klamry raczej w taki sposób: 

public void someMethod()
{
     //body
}

Eclipse robi to w ten "drugi" sposób, dlatego są takie nieścisłości, bo po prostu zapominam czasami sprzątnąć po sobie autoformatem. Dodatkowe linie natomiast pomagają mi jak chcę sobie coś poprawić w napisanym już kodzie, A uzywanie skrótu jeszcze nie weszło mi w krew 

---

Spoko, polecam jednak uzywanie klamer typu 

funkcja(){
   ...
}

To jest jednak niepisany standard Javy i nigdy nie spotkalem sie z projektem, w ktorym ktos by inaczej pisal. Gdy bedziesz robil projekt komercyjny tez bedziesz musial tak pisac. Z drugiej strony gdy mialem za zadanie napisac cos w jezyku w ktorym klamry byly stawiane inaczej po prostu to akceptowalem i pisalem wedlug zasad danego jezyka, wiec wiem ze przestawienie sie to nie jest problem. 

---

Mus to mus, rzeczywiście, dlatego, że póki co sam jestem sobie żaglem, sterem i okrętem to nie pomyslalem, że w sumie kiedyś i tak będzie trzeba przestawić się pod grupę :p Dzięki bardzo!