Minesweeper

[Oliver530]

Hallo zusammen,

ich habe jetzt mein erstes selbst programmiertes Spiel so ziemlich fertig. Ein paar Bugs sind noch vorhanden, die werden noch ausgemerzt. 🙂

Bin mit der Optik schon sehr zufrieden, aber an der Performance hängt's irgendwie noch ein wenig... 😳 Ich vermute mal es liegt an meinem Aufdeckalgorithmus.

Habt ihr eine Idee, wie ich diese unschöne Passage ändern könnte:
(Methode fragt ihre benachbarten Buttons/Felder, ob eine Mine dort liegt und ermittelt so wieviele Nachbarminen der Button hat (Zustand9 entspricht einer Mine und Zustand12 einer gesetzen Flagge auf einer Mine)). Die ganzen Exception-Fresser finde ich doch ziemlich unschön und würde sie gerne vermeiden.
Hab's auch schon statt der "try-catch-Methode" mit einer if-Abfrage vor jeder Zeile versucht. Ist aber auch nicht performanter...
Ich weiß nicht, ob ihr noch mehr Code braucht. Einfach melden. 🙂

Aufdeck-Algorithmus:

public void minenSuche(JButtonZustand [] [] grid) {
		int anzahlMinen = 0;
		try {
			if ( grid[x-1][y].getZustand() == 9 || grid[x-1][y].getZustand() == 12) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x-1][y-1].getZustand() == 9 || grid[x-1][y-1].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x][y-1].getZustand() == 9 || grid[x][y-1].getZustand() == 12) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y-1].getZustand() == 9 || grid[x+1][y-1].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y].getZustand() == 9 || grid[x+1][y].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y+1].getZustand() == 9 || grid[x+1][y+1].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x][y+1].getZustand() == 9 || grid[x][y+1].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x-1][y+1].getZustand() == 9 || grid[x-1][y+1].getZustand() == 12 ) {
				anzahlMinen++;
			}
		} catch (ArrayIndexOutOfBoundsException e) {}

public void rundOeffnen(JButtonZustand [] [] grid) {
		
		try {
			if ( grid[x-1][y].getZustand() == 10 || grid[x-1][y].getZustand() == 9 ) {
				grid[x-1][y].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x-1][y-1].getZustand() == 10 || grid[x-1][y-1].getZustand() == 9 ) {
				grid[x-1][y-1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x][y-1].getZustand() == 10 || grid[x][y-1].getZustand() == 9 ) {
				grid[x][y-1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y-1].getZustand() == 10 || grid[x+1][y-1].getZustand() == 9 ) {
				grid[x+1][y-1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y].getZustand() == 10 || grid[x+1][y].getZustand() == 9 ) {
				grid[x+1][y].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x+1][y+1].getZustand() == 10 || grid[x+1][y+1].getZustand() == 9 ) {
				grid[x+1][y+1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x][y+1].getZustand() == 10 || grid[x][y+1].getZustand() == 9 ) {
				grid[x][y+1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}
		try {
			if ( grid[x-1][y+1].getZustand() == 10 || grid[x-1][y+1].getZustand() == 9 ) {
				grid[x-1][y+1].doClick();
			}
		} catch (ArrayIndexOutOfBoundsException e) {}

	}

Hier zwecks Interesse noch die lauffähige .jar:
MinesweeperBETA.jar - Dropbox-Link

Über Lob und verbesserungsvorschläge bin ich immer sehr dankbar! 😉

 

[SlaterB]

catch (ArrayIndexOutOfBoundsException e) kannst du schonmal für alle Programme der Welt streichen,
besonders wenn du auf Performance wert legst,
wann immer irgendwo ein Array-Zugriff vorliegt, kannst du vorher selber die Grenzen prüfen,
das ist rein technisch immer schneller,
abgesehen davon dass es fachlich angebracht ist,


der Code ist im weiteren allein nach Lesbarkeit zu optimieren,
Performance sollte nicht schlecht sein, wenn du nicht Probleme selber extra einbaust,
an ein paar ifs, selbst mit unnötigen catches, kann es kaum liegen, also am geposteten Code jedenfalls nicht zu erkennen,
das Jar werde ich kaum komplett anschauen..,
kannst ja z.B. Zähler einbauen, wenn auf einem Feld mit 300 Feldern 1 Mio. mal irgendwas geprüft wird, dann läuft wohl etwas falsch

zur normalen Code-Optimierung:
> grid[x-1][y].getZustand() == 9 || grid[x-1][y].getZustand() == 12
> grid[x-1][y].getZustand() == 10 || grid[x-1][y].getZustand() == 9

soetwas zig mal wiederholen ist irrsinnig, schon einmal praktisch zu viel, da man als Tippfehler abweichende Koordinate wählen könnte
schreibe grid[x-1][y].isMine() oder was immer hier logisch geprüft wird, was bedeuten die Zustände?
notfalls einfach grid[x-1][y].isZustand9Or12()...

für das Ablaufen der Nachbarfelder erstelle die eine Klasse Abweichung oder so, einfacher Point geht auch, mit x/y,
erstelle eine Liste, fülle sie in 9 Codezeilen mit
-1,0
-1,-1
0,-1
usw., genau die benötigten Abweichungen von x und y,

ein neuer Zwischenstand könnte so aussehen:

public void rundOeffnen(JButtonZustand [] [] grid) { 
// das elementare Grid wirklich als Parameter nötig, nicht allgemein bekannt?

  for (Abweichung a : arrayAbweichungen) {
     JButtonZustand b = getNachbar(x,y,a,grid); // prüft Grenzen, kann null zurückgeben,
     // x,y sind anscheinend Instanzattribute, müssten an eine lokale Methode wahrscheinlich
     // nicht übergeben werden, grid bei vielen Methoden besser auch allgemein bekannt

     if (b == null) continue; // Grenze, nix zu tun
     if (b.isRundOderSo()) {
         b.doClick(); // Mehrfachklick nicht schlimm? oder Abbruch hier, fertig?
     }
  }
}

wenn die Zustandprüfung auf 10 und 9 nur an dieser Stelle benötigt wird, dann kann die Methode auch wieder weg
und hier der Zustand angeschaut werden,
allein schon dass eine kurze Variable b statt dreimal kompliziert grid[x+1][y] usw. steht, ist was wert,
getZustand() selber kann auch in eine Variable z, die dann zweimal verwendet wird usw.

-----

eine naheliegende Alternative wäre auch, die übers ganze Programm immer festen Nachbarn eines Punktes einmalig zu sammeln und einfach über diese zu iterieren,
statt indirekt mit 'Abweichung' zu holen, das ist jetzt im Nachhinein wohl besser,
dann wäre das Grid auch nur einmalig bei der init-Methode nötig, nicht bei jeder Runde in Methoden wie dieser

-----

soweit zum Code-Desaster


> Hab's auch schon statt der "try-catch-Methode" mit einer if-Abfrage vor jeder Zeile versucht. Ist aber auch nicht performanter...

ok, selber gesehen

> Ich weiß nicht, ob ihr noch mehr Code braucht. Einfach melden.

tja, lieber selber herausfinden..
meine angesprochenden Punkte sollten da auch nicht groß etwas ändern bisher

 

[Firephoenix]

2 Anmerkungen zum Quellcode:

catch (ArrayIndexOutOfBoundsException e) {}

ist Böse hoch 2 🙂
- Exceptions niemals verschlucken sondern mindestens den stack ausgeben (e.printStackTrace())
- Eine OOB-Exception ist zu 99.999% Fehler des Programmierers und weißt auf einen Bug hin - gerade diese Exceptions dürfen bis ganz nach oben durchknallen hauptsache man bemerkt sie früh, behandeln tut man in der Regel die Exceptions die wirklich durch Fehler zur Laufzeit auftreten (falsche eingabe, ungültige eingabedatei etc.)

grid[x-1][y+1].getZustand() == 9 || grid[x-1][y+1].getZustand() == 12 )

die zeile sieht man optisch schon dauernd, das schreit förmlich nach einer eigenen Methode.

public boolean hasGridPositionState(int x, int y, int state){
return grid[x][y].getZustand() == state;
}

daraus dann optional noch

public boolean hasGridPositionOneOfStates(int x, int y, int... state){
for(int i = 0; i < state.length; i++){
if(hasGridPositionState(x,y,state[i])){
return true;
}
}
return false;
}

dann wird aus dem Aufruf

grid[x-1][y+1].getZustand() == 9 || grid[x-1][y+1].getZustand() == 12 )

hasGridPositionOneOfStates(x-1,y+1,9,12)

die States könntest du dann auch noch auslagern, z.B. in lokale variablen oder besser gleich ein Enum:

private static final int SOME_STATE = 9;
private static final int SOME_OTHER_STATE = 12;
hasGridPositionOneOfStates(x-1,y+1,SOME_STATE,SOME_OTHER_STATE)

damit dürfte das ganze wesentlich lesbarer werden, evtl findest du ja sogar noch eine Möglichkeit das ganze dann noch kleiner zu machen 🙂

[EDIT]Code ist komplett ungetestet und einfach hier eingetippt - keine Haftung für Fehler 🙂[/EDIT]
[EDIT]SlaterB war schneller 🙂[/EDIT]

Gruß

 

[Oliver530]

Vielen Dank für die super Antworten! 🙂
Hab meine Logik jetzt ziemlich umgeschrieben und es ist jetzt auch viel übersichtlicher geworden.

Meine Exceptionfresser hab ich wie folgt gelöst:

private void aufdeckAlgorithmus(JButtonZustand [] [] grid) {
		if ( getterX() > 0 ) {
			if ( grid[x-1][y].getUnbekannt()) {
				grid[x-1][y].aufdecken();
			}
		}
		if ( getterX() > 0 && getterY() > 0) {
			if ( grid[x-1][y-1].getUnbekannt() ) {
				grid[x-1][y-1].aufdecken();
			}
		}
		if ( getterY() > 0) {
			if ( grid[x][y-1].getUnbekannt() ) {
				grid[x][y-1].aufdecken();
			}
		}
		if ( getterX() < (grid.length -1) && getterY() > 0) {
			if ( grid[x+1][y-1].getUnbekannt() ) {
				grid[x+1][y-1].aufdecken();
			}
		}
		if ( getterX() < (grid.length-1) ) {
			if ( grid[x+1][y].getUnbekannt() ) {
				grid[x+1][y].aufdecken();
			}
		}
		if ( getterX() < (grid.length-1) && getterY() < (grid[0].length-1) ) {
			if ( grid[x+1][y+1].getUnbekannt() ) {
				grid[x+1][y+1].aufdecken();
			}
		}
		if ( getterY() < (grid[0].length-1) ) {
			if ( grid[x][y+1].getUnbekannt() ) {
				grid[x][y+1].aufdecken();
			}
		}
		if ( getterX() > 0 && getterY() < (grid[0].length-1) ) {
			if ( grid[x-1][y+1].getUnbekannt() ) {
				grid[x-1][y+1].aufdecken();
			}
		}
	}

Und diese Vereinfachung hat's auch viel übersichtlicher gemacht:

temp = grid[x-1][y];
            if ( (temp.getUnbekannt() || temp.istMine()) && !temp.getFlagge() ) {
            	System.out.println("Fall 1. ");
                temp.aufdecken();

Danke! 🙂

PS: Das Performance-Problem hab ich gelöst, indem ich vor Spielstart den Buttons schon ihre Zustände (wieviele Nachbarminen) verraten habe und nicht erst suk*zes*si*ve während der Laufzeit.

 

[SlaterB]

was soll denn getterX() sein? wird doch wohl x zurückgeben?
dafür eine Methode und dann direkt dahinter für den Array-Zugriff wieder x/y, das ist ja irrsinnig,
schreibe einfach x, viel kürzer,

die Bedingungen aufs Maximum, z.B. "if y < grid[0].length-1", hast du jeweils 3x, immerhin korrekt,
analog zu meinen bisherigen Ausführungen muss ich aber wieder deutlich hinweisen dass es einfach unnötig ist,
das so oft zu schreiben, besonders falls es wieder noch mehr Methoden mit weiteren nx 3x Vorkommen gibt

du kannst auch "if yOk()" programmieren, mit einer Methode die das richtige macht,
keine Angst vor Minimethoden mit nur einer simplen Zeile drin, besser als so (relativ, aber doch schon) komplizierte Formeln wie
"y < grid[0].length-1" mehrmals aufzuschreiben, gerade direkt hintereinander weg

freilich wäre es mit Übergabe des grids als Parameter noch unschöner als nur yOk(), das grid wie gesagt Instanzattribut

-------

aber eigentlich brauch ich ja nicht reden, wenn du den Hauptpunkt, die komplexe Einzelaufzählung der 8 Umgebungsfelder weiter so unschön drinhast,
siehe meine Posting dazu, es ist gesagt was zu sagen ist,
mal abgesehen davon dass die Nachbar-Variante etwas kurz kommt obwohl sie besser ist, mündet aber in ähnlicher Schleife,
sicherlich der wichtigste Optimierungspunkt in diesem Thema,
für eine Methode schon mehr als genug lohnend,
wenn du aber wirklich zwei oder noch mehr Methoden hast, die alle 8 Nachbarn durchlaufen, dann ist die Einzelauflistung noch mehr abzulegen

wenn du nicht willst dann nicht,
aber ich nehme die Gelegenheit wahr zu betonen, dass dies aus meiner Sicht das wichtigste ist, was du hier bessern könntest

 

[Oliver530]

Danke für deine Hilfe SlaterB!

Bin gerade dabei deine Punkte umzusetzen. Bin mir aber noch nicht sicher wie ich das hier machen soll:

freilich wäre es mit Übergabe des grids als Parameter noch unschöner als nur yOk(), das grid wie gesagt Instanzattribut

ich habe mein grid als Instanzattribut in einer anderen Klasse. Die referenz muss ich dann ja aber trotzdem an jedes yOk() weiter geben? Quasi:
if ( x > 0 && yOk(MinesweeperGUi.grid) ) ...
macht das ganze dann ja auch ziemlich lang und unschön. Aber ich bin mir fast sicher, dass ich deinen Tipp noch nicht ganz verstanden hab ^^

Bin für deine Hilfe sehr dankbar!
Grüße

Edit: Achso, das pack ich ja dann in die Methode rein oder? 🙂

private boolean yOK() {
		return (y < (MainMinesweeper.game1.grid[0].length-1));
	}

So dann:

private void aufdeckAlgorithmus(JButtonZustand [] [] grid) {
		if ( x > 0 ) {
			if ( grid[x-1][y].getUnbekannt()) {
				grid[x-1][y].aufdecken();
			}
		}
		if ( x > 0 && y > 0) {
			if ( grid[x-1][y-1].getUnbekannt() ) {
				grid[x-1][y-1].aufdecken();
			}
		}
		if ( y > 0) {
			if ( grid[x][y-1].getUnbekannt() ) {
				grid[x][y-1].aufdecken();
			}
		}
		if ( xOK() && y > 0) {
			if ( grid[x+1][y-1].getUnbekannt() ) {
				grid[x+1][y-1].aufdecken();
			}
		}
		if ( xOK() ) {
			if ( grid[x+1][y].getUnbekannt() ) {
				grid[x+1][y].aufdecken();
			}
		}
		if ( xOK() && yOK() ) {
			if ( grid[x+1][y+1].getUnbekannt() ) {
				grid[x+1][y+1].aufdecken();
			}
		}
		if ( yOK() ) {
			if ( grid[x][y+1].getUnbekannt() ) {
				grid[x][y+1].aufdecken();
			}
		}
		if ( x > 0 && yOK() ) {
			if ( grid[x-1][y+1].getUnbekannt() ) {
				grid[x-1][y+1].aufdecken();
			}
		}
	}

 

[SlaterB]

so zum Beispiel, ich gebe zu übermäßig wichtig ist das bei diesem Punkt nicht,
der Methodenname war auch nur in Empörung gewählt, sollte schon etwas beschreibender und dann länger sein,
isNotXAtEdge() oder so,

das andere wäre wichtiger, dann fällt das eh weg/ wird auf nur eine Codestelle reduziert 😉