Space shooter game - kod do oceny

Jak w temacie.
Trochę ponad tydzień temu postanowiłem zacząć uczyć się Java, i zrobiłem małą gierkę na deskop, "na rozruch" i liźnięcie języka.
Można po tym jechać ile chcecie;], przyjmę każdą radę, bo projekt powstał po obejrzeniu bodajże dwóch tutorialów youtube na temat Java od użytkownika Derek Banas. Coś tam w C++ potrafię napisać, więc, uzupełniając swoją wiedzę o mnóstwo informacji ze stackoverflow, byłem w stanie to dokończyć i dopracować elementy które chciałem mieć tam zawarte. Pisana w Eclipse.
Warstwa UI oddzielona od logiki. Same obiekty ( gracz, pociski itp) przechowują tylko informacje o sobie a obliczenia do nich robione, za pomocą klas nazwaModel.

Gierka sama w sobie prosta. Standardowy shooter, gdzie poruszamy się statkiem, strzelamy kilkoma rodzajami broni (blaster, rakiety samonaprowadzające, laser, bomba), unikamy wrogów, ich pocisków oraz szybko spadających asteroid, zbieramy bonusy i próbujemy dojść do jak najwyższego poziomu ( poziom trudności wzrasta zależnie od levelu gry).
Poruszanie strzałkami. Strzelanie automatycznie, lub po wyłączeniu, spacją. Zmiany broni QWER.

https://github.com/GregoryIwanek/Space-Shooter

PS: nie dodawałem żadnych paczek instalacyjnych na gita, ponieważ ostatnim razem mi to odradzono. To co tam jest, to skompilowany projekt z Eclipse.

• Duża duplikacja kodu. Masz w różnych klasach metody typu getSpeed, setImage, które są takie same albo podobne w różnych klasach. Czyli słabe reużycie kodu. Nową klasę piszesz i robisz od nowa takie same metody.

Mógłbyś tego uniknąć tworząc np. jakąś wspólną klasę bazową dla obiektów gry (swoją drogą nie rozumiem, czemu obiekty gry dziedziczą z klasy Rectangle? To, że dany obiekt gry jest prostokątem to raczej powinno być szczegółem implementacyjnym. Jak będziesz chciał zrobić obiekty w kształcie kółek to jesteś w d...)

• Słaba abstrakcja. Nie wiem czy to dobre słowo, ale chodzi mi o to, że traktujesz wszystko zbyt konkretnie, zamiast operować na abstrakcjach. Tzn. zamiast zrobić listę obiektów klasy GameObject (czy jakbyś to sobie nazwał) i wszystkie obiekty (gracza, kule, wroga) traktować tak samo, to robisz osobne klasy do wszystkiego; osobną zmienną na gracza; osobną listę na wrogów, którą rysujesz osobno od innych klas:

	for (Enemy enemy : listOfEnemyShips)
		{
			g2.drawImage(enemy.getImage(), enemy.getLocation().x, enemy.getLocation().y,
					enemy.getSize().width, enemy.getSize().height, null);
		}

Tak samo masz osobną listę na kule i też osobno przelatujesz. Takie zachowanie (trzymanie różnych obiektów gry osobno) ma czasami sens, ale tutaj wygląda raczej na przypadek:

		//paint enemy bullets
		for (Bullet bullet : listOfEnemyBullets)
		{
			g2.fillRect(bullet.getLocation().x, bullet.getLocation().y, bullet.getSize().width, bullet.getSize().height);
		}

I w innych miejscach podobne rzeczy - traktujesz każdy obiekt (gracz, kule, wroga, bonus itp.) jako coś osobnego i do każdej klasy robisz nowe metody, każdą klasę inaczej obsługujesz (mimo, że prawie tak samo). Tym sposobem każdy nowy typ obiektu generuje niepotrzebny kod. Czyli słabo zaprojektowane to wszystko jest.

LukeJL napisał(a):

Mógłbyś tego uniknąć tworząc np. jakąś wspólną klasę bazową dla obiektów gry (swoją drogą nie rozumiem, czemu obiekty gry dziedziczą z klasy Rectangle? To, że dany obiekt gry jest prostokątem to raczej powinno być szczegółem implementacyjnym.

Zastanawiałem się nad tym, ale zrezygnowałem z tego, bo mimo tego, że obiekty są podobne, mają całkowicie inne zadania oraz atrybuty, i rozbijałoby się to o to. Wyszłaby jedna mega klasa z multim metod do wszystkich przypadków zastosowań. Myślałem, żeby zrobić jakąś klasę abstrakcyjną do obsługi powtarzających się metod ( jak np. setImage), ale postanowiłem to wstawić najpierw tutaj.
Możesz rozwinąć o co chodzi z tym Rectangle? Wybrałem to sobie bez większego zastanowienia, ponieważ ma od razu wbudowane użyteczne metody dla obiektów w takiej gierce (location, size itp).

LukeJL napisał(a):

Tzn. zamiast zrobić listę obiektów klasy GameObject (czy jakbyś to sobie nazwał) i wszystkie obiekty (gracza, kule, wroga) traktować tak samo, to robisz osobne klasy do wszystkiego; osobną zmienną na gracza; osobną listę na wrogów, którą rysujesz osobno od innych klas.

Naprawdę, jeżeli umieścił bym to wszystko w jednej klasie to ktoś by się tego czepnął z kolei:). Chociaż posiadanie jednego typu obiektów załatwiłoby wstawiony przez Ciebie fragment kodu ( nawiasem mówiąc, pytałem tutaj wcześniej czy dałoby radę obejść to na zasadzie czegoś jak template w C++ i przekazania tych list do tej metody-> z obiektów z list wyciągane są te same dane, ale dostałem odpowiedź, że raczej nie, albo, że nie warte byłoby to próbowania) i wystarczyłaby jedna lista z obiektem typu GameObject, zamiast kilku na każdy rodzaj.
Dzięki za uwagi.

Naprawdę, jeżeli umieścił bym to wszystko w jednej klasie to ktoś by się tego czepnął z kolei:)

dlaczego w jednej klasie?
---------- nadklasa GameObject ---------------
| | | |
Enemy Player Bullet Bonus

I tak dzięki prostemu dziedziczeniu masz klasę, która zawiera wspólne elementy oraz podklasy szczegółowe. (z drugiej strony dziedziczenie ma swoje wady, więc często zamiast dziedziczenia stosuje się kompozycję obiektów - np. zamiast dziedziczyć z klasy Rectangle, mógłbyś zrobić tak, że klasa zawiera pole klasy Rectangle).

Poza tym - co te twoje klasy robią? Przecież to i tak w większości zwykłe pojemniki na zmienne, a cała logika gry jest w tym pliku z tego co widzę:
https://github.com/GregoryIwanek/Space-Shooter/blob/master/src/ModelPackage/GameModel.java
Równie dobrze prawie wszystkie pola w klasach Enemy, Bullet etc mogłyby być publiczne i nie bawić się w gettery i settery, bo na to samo by wyszło. Albo zrobić wszystko na jakichś haszmapach. Tj. nie twierdzę, że to lepszy pomysł, tylko pod kątem organizacji kodu/designu projektu/ na to samo by wyszło.

Z takim dziedziczeniem to przyznam, że dobry pomysł. Nie pomyślałem o tym z tej strony, żeby dodać jakąś swoją nadklasę i dziedziczenie po niej dla tych obiektów. Przyznaję Ci rację i zmienię w tym kierunku. Dzięki.
W ten sposób mógłbym wywalić kilka powtarzających się metod z obiektów. Nie byłoby ich jakoś dużo, ale zawsze coś.

Z tymi getterami/setterami, to sam wiesz... niby wszędzie czytałem, że zmienne publiczne to "zuo" i ogólnie herezja. Na pewno byłoby prościej/krócej bez nich, ale skoro już są, to zostawię. Nie bawiłem się też tutaj w żadne testy ( temat do lepszego ogarnięcia), oprócz imput/output za pomocą System.out.println(), ale na koniec zawsze to usuwam, żeby nie robić sobie burdelu w konsoli w trakcie odpalania programu.

I jak ogólnie to to wygląda? Dno i wodorosty, czy jakoś ujdzie? Chodzi mi ogólnie o składnię/formatowanie/nazewnictwo, wykorzystane elementy języka, czy nie za dużo wrzucam do metod, albo czy za bardzo się z nimi rozdrobniłem? Pytań dużo, ale po to, żeby rzucić pomysł o komentarz:). Jak na razie to poziom hobbystyczny.

Teraz zabieram się za przerobienie i analizę wszystkich tutorialii z http://www.journaldev.com/2888/spring-tutorial-spring-core-tutorial i pewnie po zakończeniu, napiszę coś dla Spring+Hibernate, jakiegoś CRUDa albo coś podobnego. Mam listę zagadnień które muszę ogarnąć do pierwszej roboty i jeszcze dwa miesiące czasu, kiedy jestem uziemiony w mieszkaniu z pewnych powodów, tak więc mogę poświęcać czas na ogarnianie tematu.

Z tymi getterami/setterami, to sam wiesz... niby wszędzie czytałem, że zmienne publiczne to "zuo" i ogólnie herezja.

Zasady są dobre, jeśli się je rozumie. A żeby zrozumieć zasady trzeba często je złamać (w zasadzie nawet nie złamać - przecież te wszystkie zasady i tak powstały w ten sposób, że ludzie pisali bez zasad, zobaczyli, że to kiepskie, więc wymyślili sobie zasady, żeby uniknąć błędów w przeszłości).

Zmienne publiczne są o tyle złe, że przez to inne obiekty mogą wpływać na szczegóły innego obiektu. Np. załóżmy, że obiekt A ma zmienną image. Ta zmienna (zakładając enkapsulację) nie powinna być ruszana przez inne obiekty. Zmienna image to powinna być prywatna sprawa obiektu A (jeśli istnieje potrzeba, żeby zmienić obrazek to obiekt powinien sam o tym decydować, być odpowiedzialnym za własną zmienną)

I teraz - czemu obiekt B nie powinien zmieniać pola obiektu A? (czyli czemu zmienne publiczne to "zuo")? Ponieważ łatwo wtedy o burdel, przestajesz mieć kontrolę nad tym, który obiekt zmienia co. Obiekt A ma jakąś zmienną, a jak pozwolisz wszystkim innym obiektom zmieniać tę zmienną, to potem bardzo łatwo o bugi. Z tego samego względu zmienne globalne są "złe" (uogólniając: "shared mutable state is the root of all evil" jak to się czasami mówi).

Problem tylko w tym, że dając setter do pola image

	public void setImage(BufferedImage image)
	{
		this.image = image;
	}

robisz taki jakby "back door" i inne obiekty mogą ruszać zmienną image, która wcale nie jest prywatna, jeśli jakiś inny obiekt może sobie wywołać funkcję setImage i zmienić image. Co z tego, że zmienna jest formalnie prywatna jak i tak pozwalasz zmieniać stan obiektu każdej innej zmiennej? I każdy obiekt może odczytać bieżącą wartość zmiennej? Tym sposobem stan obiektu staje się współdzielony (bo to docelowo bardziej o stan obiektu chodzi, a nie o zmienne). Więc możesz mieć takie same bugi/problemy potem, jakbyś dał zmienną publiczną.

Więc: kierujesz się zasadą, której nie rozumiesz do końca - i nic nie zyskujesz, a jedynie tracisz o tyle, że twój kod staje się dłuższy.

Oczywiście, gettery i settery przydają się w wielu przypadkach i nie chcę demonizować tego wzorca, jednak każdy wzorzec należy stosować ze zrozumieniem. Gettery i settery mają pewne oczywiste zalety (tj. kontrola dostępu, możliwość odpalenia dodatkowych przeliczeń itp.), ale samo korzystanie z getterów/setterów wcale nic nie zapewnia, ani nie sprawia, że twój kod stanie się lepszy czy bardziej odporny na bugi...

Tak samo zmienne publiczne - niby zło i herezja, ale jak się wie co się robi, to można stosować zmienne publiczne w ten sposób, że nic złego się nie będzie dziać (główny problem jednak polega na tym, że trzeba rozumieć dlaczego coś może być złe, a to często łączy się z tym, że człowiek na własnej skórze się pewnych rzeczy uczy i dopiero potem wie, czego unikać i na co zwracać uwagę).

Tak samo np. ze wzorcem Singleton, też niby "zły", ale jednak czasem można go zastosować i nic się nie dzieje. Trzeba po prostu rozumieć dlaczego coś może być złe. (tutaj np. Wujek Bob fajnie broni wysmiewanego wzorca Singleton: https://8thlight.com/blog/uncle-bob/2015/06/30/the-little-singleton.html - przy czym esencją tego tekstu wcale nie jest to, że Singletony są "dobre", tylko, że zasady trzeba rozumieć, a nie przyjmować na dobrą wiarę).