Cześć,
właśnie ukończyłem swoją pierwszą aplikację. Będę bardzo wdzięczny za ocenę kodu i wskazówki co powinienem poprawić, aby aplikacja była napisana czyściej i lepiej. W większości działania opierają się na warunkach i mimo, że gra działa, to mam wrażenie że nie do końca tak powinno to wyglądać.
Z góry bardzo dziękuję za poświęcony przez Was czas.
PS. GitHuba jeszcze nie bardzo ogarniam, ale mam nadzieję, że to co tam jest pozwoli na ocenę kodu.
Rzuciłem okiem i mam kilka uwag:
Bardzo dziękuję Kamil, wszystkie rady biorę od razu na warsztat. Mam jedynie pytanie co do tych nawiasów. Jeśli dobrze rozumiem masz na myśli, że raz otwieram nawias na końcu linii, a w innym miejscu w kolejnej. Ujednolicenie nie będzie problemem, tylko zastanawia mnie ostatnie zdanie "zdecyduj mądrze pamiętając, że Java to nie C, C++ ani C#". Mógłbym Cię prosić o rozwinięcie? Niestety nie znam tych języków, Java to mój pierwszy kontakt z programowaniem i nie bardzo rozumiem różnicę między jednym sposobem, a drugim. :-)
Przemyślałbym kwestię nazewnictwa. Nazwa metody powinna mieć formę czasownikową i mieć wydźwięk komendy, wyjaśniającej, jaki będzie efekt heh wywołania (np. printLine). Ewentualnie zapytania, wyjaśniającego, jaka wartość zostanie zwrócona (np. getPassword). Twoje metody nazywają się zaś np. newComponents albo isEmpty. Ta druga miałaby jak najbardziej sens, gdyby należała do tej drugiej grupy i zwracała wartość (tzn. odpowiadała, czy coś rzeczywiście jest empty). Tak jednak nie jest, a jako komenda nie ma sensu.
Taki kod jest trudny do zrozumienia. To tak, jakbym zostawił na lodówkę kartkę, przypominającą mi o zadaniach na dziś, i pisał np. tylko "pies". Ja pamiętam, o co mi chodziło, ale żona mogłaby już nie wiedzieć, czy miałem na myśli, że trzeba go wykąpać, wziąć do weterynarza, nakarmić czy wyprowadzić.
Warto by też położyć większy nacisk na logikę i zwięzłość:
Java if (word.getPassword().length == 0) {
play.setEnabled(false);
} else if(word.getPassword().length != 0) {
play.setEnabled(true);
}
word.getPassword() nie może mieć równocześnie długości 0 i innej niż 0. Nie trzeba zatem dwa razy sprawdzać tego samego - chyba, że nie wiemy, co robi else.
Wystarczyłoby:
if (word.getPassword().length == 0) {
play.setEnabled(false);
} else {
play.setEnabled(true);
}
Ale przecież skoro przekazywana do setEnabled wartość jest wprost zależna od wartości warunku, który sprawdzamy, to wystarczyłoby całą tę konstrukcję zwinąć do jednej linii i zapisać po prostu tak:
play.setEnabled(word.getPassword().length > 0);
Ewentualnie, dla większej czytelności:
boolean wordNotEmpty = word.getPassword().length > 0;
play.setEnabled(wordNotEmpty);
Ogólnie wydaje mi się, że następną umiejętnością, o jaką powinieneś się pokusić, jest unikanie duplikowania kodu. Jeśli programujemy za pomocą techniki kopiuj-wklej, to znaczy, że coś robimy nie tak. Postawiłbym sobie na twoim miejscu zadanie następujące - metody newEasyGame, newMediumGame i newHardGame są prawie w 100% identyczne. Należałoby zredukować je do jednej metody startGame, która obejmowałaby kod wspólny dla tych trzech metod - a to, czym te metody się różniły, przekazywać do tej uwspólnionej za pomocą parametrów.
PS. Nie rozumiem także potrzeby używania zbędnych nawiasów, np. czemu (Main.health) = 5 zamiast po prostu Main.health = 5. To oczywiście drobiazg