Code Review Änderung Anwenden

[Muesli1]

Code Review:

Allgemeines (subjektiv):

• var zu Benutzen ist in meinen Augen generell eine schlechte Angewohnheit in Java. Aber das kann natürlich jeder selbst entscheiden. Trotzdem würde ich den Studierenden lieber keinen Code mit var als "guten Stil" zeigen.

Benennungen:

• GameController sollte "GameSceneController" heißen, da er im wichtigsten Sinne ein SceneController ist
• Die Klasse ControlledScene sollte im package "h13.controller.scene" statt "h13.view.gui" liegen, oder?

Scenen & So:

SceneSwitcher:

• Ein SceneController wird entweder über FXMLLoader erstellt (AboutController, HighscoreController usw) oder direkt per Hand (GameController). Diese Uneinstimmigkeit ist unschön und könnte vielleicht Probleme verursachen.
• Da eine (von FXMLLoader erstellte) Scene keine Referenz auf ihren Controller hat (soweit ich verstanden habe), könntest du auch für alle Scenen die Controller selbst erstellen, anstatt es von FXMLLoader machen zu lassen.
• Die Methoden "loadXXXXScene" sind nicht sehr schön und sollten eigentlich durch eine Methode mit Enum ersetzt werden. Aber für Test-Zwecke sind wohl unterschiedliche Methoden besser. Jedoch sollten die genutzen Pfade unbedingt Konstanten werden, anstatt hardcoded Strings.
• Die getStage-Methode ist sehr ugly und wird benötigt, weil sich die SceneController nicht ihre Stage merken. Das könnten die SceneController jedoch ohne Probleme tun, weil diese mit initStage ihre jeweilige Stage bekommen, wenn ich da nichts übersehen habe.
• Ansonsten sollte die getStage-Methode in die Klasse SceneController geschoben werden, weil sie nur dort verwendet wird und eigentlich nichts mit SceneSwitcher zu tun hat.
• Mir gefällt ein "static" Scene Switcher eigentlich nicht, aber da JavaFX die Scenen-Logik so behandelt ist das hier natürlich genau richtig

SceneController:

• Da alle Klassen, die von SceneController erben, den Titel der Stage setzen, sollte man diese Funktionalität vielleicht direkt in SceneController implementieren und stattdessen eine abstrakte Methode wie "abstract String getSceneName()" deklarieren.
• Hier dasselbe Argumente für die verschiedenen "loadXXXXScene"-Methoden wie bei SceneSwitcher

ApplicationSettings:

• Die Speicherung der Highscores und anderen Settings in einer static Klasse ist für mich unangenehm. Ich hätte lieber eine Referenz, die zu allen Methoden getragen wird, die das benötigen. Oder alternativ eine Haupt-Klasse, welche die Settings verwaltet auf die jeder Zugriff hat. Aber da JavaFX generell auch diesen Ansatz folgt, ist das wohl auch korrekt hier. Wollte ich nur erwähnen.
• Das Attribut "highscores" sollte private sein und seinen eigener Getter bekommen. Ansonsten passt es nicht zu den sonstigen Stil von den properties in der Klasse
• Es sollte am besten eine Methode hinzugefügt werden, um einen Eintrag in highscores hinzuzufügen, anstatt es irgendwo in einer anderen Klasse zu machen, da hier die zentrale Verwaltung ist.

Main Menu:

• Das Laden von Scenes direkt über den Aufruf der geerbten Methoden von SceneSwitcher ist natürlich sehr clever.
• Alles super!

About:

• Auch alles super
• Auch wenn sich hier die extra Klasse für AboutController sehr redundant anfühlt, aber fürs Konzept natürlich notwendig ist.

Highscores:

• Hier fehlt natürlich noch das Einlesen und Speichern aus einer Datei, falls das noch dabei sein soll
• Ansonsten super, hier sieht man gut die Stärken von JavaFX

Settings:

• Hier ist die bidrectional Binding Logik sehr elegant
• Die Full-Screen Einstellung funktioniert jedoch nur in der GameScene? Ist das beabsichtigt?
• Und beim ESC drücken in der GameScene wird auch gleichzeitg der Dialog zum Zurückkehren zum Hauptmenü ausgelöst

GameScene

• Der Konstruktor sieht sehr hacky aus. Aber wenns funktioniert passt alles
• Die GameScene hat eine Referenz auf seinen SceneController (GameController), was eine Sonderstellung ist, da das sonst bei keiner Scene der Fall ist. Das zerstört natürlich etwas die Trennung von Scene und Controller. Die GameScene braucht im Moment keine Referenz zum GameController; nur das GameBoard, also könnte man diese Beziehung aus der GameScene auch rausnehmen und nur die Referenz an das GameBoard übergeben.
  Aber die Referenz von dem GameController auf die GameScene ist natürlich notwendig.
• Man könnte vermutlich dann auch die GameScene aus "view.gui" rausnehmen und in "h13.controller.scene" packen.
• Hier sind noch redundante Attribute wie "enemyController" und "lastGameboardSize"
• Die Anweisungen am Ende von initGameboard bezüglich des GraphicsContext sehen sehr redundant aus, sind die wirklich notwendig und korrekt?

Rendering:

GameBoard:

• Die Scale-Logik sieht sehr hacky aus. Das riecht nach einem fundamentalen Problem.

• Das setzten der Y-Koordinate der Spielerposition aufgrund der Display-Größe verstärkt diesen Geruch noch mehr.

• Die Position der Sprites, das heißt der Anzeige, sollte nichts am Spiel ändern. In diesem Fall ist aber die Spielposition und die Anzeigeposition ein und dasselbe. Aus diesem Grund müssen bei Display-Größen-Änderungen die Position der Entities geändert werden.

• Das ist kein kleines Problem, sondern fatal.

• Die Anzeigeposition muss natürlich auf Basis der Skalierung und der Display-Größe berechnet werden.

• Die tatsächliche Position der Gegner und des Spielers sollte jedoch komplett unabhängig davon sein.

• Aus diesem Grund sollte die Scale in die "Sprite.render"-Methode übergeben werden, in der diese dann an die korrekte Position angezeigt wird.

• Das Fehlen dieser Trennung kann viele Game-Breaking-Bugs in verschiedenen Window-Größen verursachen.

• Ein manuelles foreach für die verschiedenen Klassen sollte vermieden werden. Hier wäre es Objektorientiert besser, eine Art int getLayer()-Methode in der Sprite-Klasse einzubauen, und z.B. 3 Layers festzulegen und dann per Loop

for(int layer = 0; layer <= 3; layer++) {
    getGameController.getSprites(s -> s.getLayer() == layer).forEach(s -> s.render(gc));
}

zu arbeiten.

• Es werden für jeden Player die Score und Lives angezeigt, jedoch alle am selben Ort. Das bedeutet, eigentlich funktioniert es nur mit einem Spieler. Warum wird die einzigartige Player-Instanz nicht im GameController gespeichert, anstatt immer und immer wieder alle Sprites nach einem Player zu filtern? Das wäre weitaus effizienter und würde auch den Code deutlich mit einer Anweisung final Player player = getGameController().getPlayer(); vereinfachen.

Sprite:

• Wie bereits schon gesagt sollte die render-Methode und das Texture-Loading am besten nicht in Sprite passieren.

• Die Methode "loadTexture" lädt jedes Mal die Bilder neu von der Datei. Diese könnten auch simpel gecached werden, zum Beispiel:

private static final Map<String, Image> imageCache = new HashMap<>();

und dann in der Methode:

texture = imageCache.computeIfAbsent(path,s -> new Image(s));

Game Logik:

Sprite:

• Warum wird überall mit DoubleProperty usw gearbeitet, statt normalen "double"s? Gibt es hierfür einen guten Grund?

• In die() wird direkt auf die sprites Liste vom GameController zugegriffen, und das während des Updates. Das kann zu Concurrent-Modification-Exceptions führen

• getPaddedPosition() ist schlecht benannt, eigentlich werden hier bounded-Positions berechnet, sodass die Sprite immer im Fenster zu sehen ist.

• Die getter() und setter() sollten am Ende oder am Anfang sein, so in der Mitte der Klasse stören sie das Lesen der Klasse enorm.

• update():
  Jedes Sprite sollte nicht selber entscheiden, was seine deltaTime ist, das sollte global festgelegt werden. Auch ist der Name "gameTick" verwirrend, weil er nicht auf Tick-Basis arbeitet.
  Das Konstrukt der GameTickParameter wirkt sehr redundant, weil die meisten Attribute nie genutzt werden.

• width & height sollten konstant sein

GameController:

-update: .map(Playerable.class::cast) ist unnötig
-gameLoop resume Logik ist kaputt, sollte resume(long) aufrufen, oder?

-init: GamePlay setzt indirekt EnemyController und PlayerController. Ist das wirklich notwendig?

• Pause & resume & stop sollte vielleicht lieber selber eingebaut werden anstatt den gameLoop AnimationTimer zu nutzen.

-reset: Alle Objekte (GamePlay, EnemeyController und PlayerController) neu zu erstellen ist etwas unclean.

GamePlay:

• Notwendig?

PlayerController:

• Key handling ist in Ordnung so

EnemyController:

• init: Hier sollte die Breite des GameBoards keine Rolle spielen, sondern die genormte Größe des Spielfelds
• Wenns funktioniert ist es gut

EnemyMovement:

• Position werden gesetzt aufgrund von movementProgress. Hier wäre eine flexiblere Bewegung z.B. mit einer Gesamt-BoundingBox aller Gegner und mit zwei Rechts-Links, Oben-Unten-Variablen besser

GameState:

• Wird nicht genutzt

Playable:

• Sollte komplett ersetzt werden durch andere Logik

SpriteType:

• Wird nicht genutzt, könnte aber zum Beispiel für Anzeige-Layer genutzt werden
  -> Explosion wird wahrscheinlich zu viel!

Enemy:

• Anstatt eine Texture zu laden sollte hier ein EnemyType oder eine ZufallsZahl gesetzt werden.

Bullet:

• Die Hit detection Logik ist sehr komplex, aber sinnvoll.
• Hier sollte statt eine List "hits" anzulegen auf "isDead" geprüft werden

BattleShip:

• Die "isFriend"-Logik mit Klassen ist unangenehm. Das sollte man vielleicht über SpriteType lösen.
• hasBullet() braucht sehr viele Resourcen. Die Bullets könnten sich auch auf eine Liste für den jeweiligen Owner zuweisen, das wäre deutlich effizienter