Codedesign und andere Kritikpunkte

[Mostly_Harmless]

Hallo,

ich bin noch relativ unerfahren in der OOP. Vor allem in Fragen Code-Design kenne ich mich noch nicht gut aus.

Daher poste ich euch hier mal den Quellcode meines vor Kurzem geschriebenen Minesweeper-Spiels, in der Hoffnung, dass der ein oder andere Zeit und Lust findet, sich ihn mal anzuschauen, und mir Verbesserungsvorschläge geben kann.
Insbesondere bei den beiden Klassen "Menu" und "GUI", die - wie zumindest der Name der zweiten Klasse vermuten lässt - für die grafische Ausgabe zuständig sind, bin ich mir unsicher was die Instanzvariablen angeht. Ob man also die einzelnen GUI-Elemente als Membervariablen speichert oder ob es da andere elegantere Möglichkeiten gibt.

Natürlich wäre ich auch über jede Kritik dankbar, die sich nicht aufs Design bezieht.

Also - was habe ich eurer Meinung nach alles besonders schlecht (oder auch besonders gut ) gelöst?

Hier der Link zum Code.

Btw, wie kann man hier Dateien an den Beitrag anhängen?

 

[SlaterB]

> Btw, wie kann man hier Dateien an den Beitrag anhängen?

ich glaub gar nicht

-------

ich habe mal ganz kurz reingeschaut und
a) keine großen Membervariablen-Wiederholungen a la
Jbutton button1
Jbutton button2
Jbutton button3
Jbutton button4
gesehen sowie
b) keine großen Code-Wiederholungen,

somit bist du schon weit über Durchschnitt von unsicheren Anfängern,
da kann der Rest eigentlich gar nicht mehr schlecht sein,

-----


was mir grob weniger gut auffiel:
Menu extends JFrame
GUI extends JFrame

hat dein Spiel zwei unterschiedliche Grafikfenster? (nicht ausprobiert)
wenns nur ein Fenster ist, dann sollte es nur eine JFrame-Klasse geben,
besonders komisch ist das bei deinem expliziten Hinweis,
dass nur die zweite Klasse GUI für Darstellung zuständig sein soll

-----

private void refreshButton( int h, int w ) {
		if ( this.mines.getMarked( h, w ) ) {
    		this.butArray[h][w].setBackground( Color.blue );
    	} else if ( ! this.mines.getVisible(h, w) ) {
        	this.butArray[h][w].setBackground( Color.darkGray );
        } else {
        	this.butArray[h][w].setBackground( this.getContentPane().getBackground() );
        	this.butArray[h][w].setEnabled( false );
            
            int val = this.mines.getValue( h, w );
            switch ( val ) {
                case 0:
                	this.butArray[h][w].setText( "" );
                    break;
                case Minefield.MINE:
                	this.butArray[h][w].setText( "M" );
                    break;
                default:
                	this.butArray[h][w].setText( "" + val );
                    break;
            }
        }

doch ein bisschen Wiederholung :
definiere dir einmal am Anfang der Methode
JButton b = this.butArray[h][w];
dann kannst du im weiteren Verlauf immer b schreiben statt 7x 'this.butArray[h][w]'

 

[Mostly_Harmless]

ich habe mal ganz kurz reingeschaut und
a) keine großen Membervariablen-Wiederholungen a la
Jbutton button1
Jbutton button2
Jbutton button3
Jbutton button4
gesehen sowie
b) keine großen Code-Wiederholungen,

Bin zwar nicht unbedingt schreibfaul, aber das wäre dann trotzdem zu viel für mich bei einer Größe von 16x30 Feldern. :lol:

somit bist du schon weit über Durchschnitt von unsicheren Anfängern,
da kann der Rest eigentlich gar nicht mehr schlecht sein,

Keine voreiligen Schlüsse ziehen bitte. ;-)

was mir grob weniger gut auffiel:
Menu extends JFrame
GUI extends JFrame

hat dein Spiel zwei unterschiedliche Grafikfenster? (nicht ausprobiert)
wenns nur ein Fenster ist, dann sollte es nur eine JFrame-Klasse geben,
besonders komisch ist das bei deinem expliziten Hinweis,
dass nur die zweite Klasse GUI für Darstellung zuständig sein soll

Ja, sind zweit unterschiedliche Fenster, von denen aber immer nur eins geöffnet ist.
Menu (hier ist auch die main-Methode) zeigt ein kleines Fenster an, in dem man die Höhe, Breite und Anzahl der Minen eingeben kann. Klickt man dann auf "Start" wird das Fenster geschlossen und es öffnet sich ein Fenster der Klasse GUI, was dann das eigentliche Spielfeld anzeigt.

doch ein bisschen Wiederholung :
definiere dir einmal am Anfang der Methode
JButton b = this.butArray[h][w];
dann kannst du im weiteren Verlauf immer b schreiben statt 7x 'this.butArray[h][w]'

Na das ist ja keine wirkliche Wiederholung in dem Sinne wie man sie vermeiden sollte denke ich. Ist halt nur weniger Schreibarbeit. ;-)
Aber ob ich jetzt 7x b schreibe oder einmal this.butArray[h][w] und das dann 6x kopiere... :lol:

 

[Andreas29]

Hi,

dabei geht es weniger um Schreibfaulheit (b sind weniger Buchstaben als die andere Lösung) sondern vielmehr um Sicherheit. Wenn du dein Array jetzt mal verändern willst musst du die Änderung in deiner Version an 7 Stellen macht. Wenn du dir eine Variable hälst, brauchst du nur eine Stelle ändern. 7 Änderungen, 7 Fehlermöglichkeiten gegen 1 Änderung 1 Fehlermöglichkeit. Je weniger Fehlermöglichkeiten, desto besser meiner bescheidenen Möglichkeit nach.

Grüße,
Andreas

 

[Mostly_Harmless]

Ok, danke, hab die Stelle jetzt entsprechend geändert.

Hat sonst noch wer mal in den Code reingeschaut und kann mir Tipps geben?

Edit: Achja, was ich noch fragen wollte: Wie kann man die Zahlen in den JButtons noch größer darstellen?
Wenn ich die Schriftgröße erhöhe, wird statt der jeweiligen Zahl nur "..." angezeigt, obwohl der JButton Platz genug hätte, um die Zahl zu zeigen. Hab noch keine Methode gefunden, mit der ich den Rand verkleinern kann, sodass der Text den gesamten JButton füllen darf.

 

[Ark]

Ich mache es generell so, dass ich eine Main-Klasse anlege, die diverse Informationen über die Software bereithält (Name, Versionsnummer, Autor usw.), über die Verwendung per Konsole informiert und vor allem die main()-Methode enthält. Bei deinen Klassen hatte ich arge Schwierigkeiten, selbst bei Syntaxhervorhebung die main()-Methode ausfindig zu machen.

Ark

EDIT:

this.easy.setActionCommand( "easy" );

//blubb

if ( cmd.equals("easy") ) {

Stell dir nur einmal vor, du wolltest "easy" in "einfach" (oder irgendetwas anderes) ändern: du müsstest es an mehreren Stellen tun. Das ist nicht so ganz das Wahre. Und noch weiter: Stell dir vor, du wolltest noch mehr Modi als "easy", "medium" usw. anbieten, vielleicht sogar so weit, dass man als Nutzer selbst "Spielbretter" speichern kann: Dann würde Code wie

            	if ( cmd.equals("easy") ) {
            		this.mode = new Mode( Mode.EASY );
            	} else if ( cmd.equals("medium") ) {
            		this.mode = new Mode( Mode.MEDIUM );
            	} else if ( cmd.equals("hard") ) {
            		this.mode = new Mode( Mode.HARD );
            	}

gegen unendlich streben. Besser ist es, bei Auswahl eines Spielbretts nur die Parameter zu setzen, als hätte man sie selbst eingegeben (so wird das ja schon gemacht), aber dann, wenn das Spielfeld berechnet werden soll, wirklich nur diese Parameter (Breite, Höhe, Minenzahl) abzunehmen, ohne wieder auf diese verschiedenen Modi zurückzugreifen.

Ark

 

[Zed]

Ok, danke, hab die Stelle jetzt entsprechend geändert.

Hat sonst noch wer mal in den Code reingeschaut und kann mir Tipps geben?

Edit: Achja, was ich noch fragen wollte: Wie kann man die Zahlen in den JButtons noch größer darstellen?
Wenn ich die Schriftgröße erhöhe, wird statt der jeweiligen Zahl nur "..." angezeigt, obwohl der JButton Platz genug hätte, um die Zahl zu zeigen. Hab noch keine Methode gefunden, mit der ich den Rand verkleinern kann, sodass der Text den gesamten JButton füllen darf.

http://java.sun.com/javase/6/docs/api/javax/swing/AbstractButton.html#setMargin(java.awt.Insets)

 

[Mostly_Harmless]

Ich mache es generell so, dass ich eine Main-Klasse anlege, die diverse Informationen über die Software bereithält (Name, Versionsnummer, Autor usw.), über die Verwendung per Konsole informiert und vor allem die main()-Methode enthält. Bei deinen Klassen hatte ich arge Schwierigkeiten, selbst bei Syntaxhervorhebung die main()-Methode ausfindig zu machen.

Ich finds zwar nicht so wichtig überall meinen Namen reinzuschreiben, aber eine Main-Klasse wäre vielleicht wirklich ganz nützlich. Zumindest bei größeren Projekten (dies ist ja noch relativ überschaubar).

this.easy.setActionCommand( "easy" );

//blubb

if ( cmd.equals("easy") ) {

Stell dir nur einmal vor, du wolltest "easy" in "einfach" (oder irgendetwas anderes) ändern: du müsstest es an mehreren Stellen tun. Das ist nicht so ganz das Wahre.

Daran hab ich auch schon gedacht, mir ist aber keine Lösung dazu eingefallen.
Außer, dass ich dafür extra noch neue Instanz- bzw. Klassenvariablen anlegen könnte, aber wird das dann nicht zuviel?

Und noch weiter: Stell dir vor, du wolltest noch mehr Modi als "easy", "medium" usw. anbieten, vielleicht sogar so weit, dass man als Nutzer selbst "Spielbretter" speichern kann: Dann würde Code wie

            	if ( cmd.equals("easy") ) {
            		this.mode = new Mode( Mode.EASY );
            	} else if ( cmd.equals("medium") ) {
            		this.mode = new Mode( Mode.MEDIUM );
            	} else if ( cmd.equals("hard") ) {
            		this.mode = new Mode( Mode.HARD );
            	}

gegen unendlich streben. Besser ist es, bei Auswahl eines Spielbretts nur die Parameter zu setzen, als hätte man sie selbst eingegeben (so wird das ja schon gemacht), aber dann, wenn das Spielfeld berechnet werden soll, wirklich nur diese Parameter (Breite, Höhe, Minenzahl) abzunehmen, ohne wieder auf diese verschiedenen Modi zurückzugreifen.

Das war auch der Grund, weshalb ich für den Schwierigkeitsgrad eigens die Klasse Mode angelegt hab.
So kann ich leicht neue vordefinierte Schwierigkeitsstufen hinzufügen. Aber abfragen, welcher Mode nun gewählt wurde und den dann dementsprechend erzeugen muss ich doch irgendwie.

Danke euch allen für die Antworten!

 

[fjord]

Ich hab jetzt nicht in deinen Code gesehen, sondern nur die paar Ausschnitte aus diesem Thread. Mir ist aber aufgefallen, dass du sehr häufig this benutzt. Wenn du innerhalb einer Klasse auf Instanz- oder Klassenvariablen oder -methoden zugreifst, musst du kein this benutzen. Ausnahme ist, wenn eine Variable durch eine lokale Variable verdeckt wird.

 

[Mostly_Harmless]

Ich hab jetzt nicht in deinen Code gesehen, sondern nur die paar Ausschnitte aus diesem Thread. Mir ist aber aufgefallen, dass du sehr häufig this benutzt. Wenn du innerhalb einer Klasse auf Instanz- oder Klassenvariablen oder -methoden zugreifst, musst du kein this benutzen. Ausnahme ist, wenn eine Variable durch eine lokale Variable verdeckt wird.

Ich weiß, dass es nicht nötig ist.
Ich machs aber trotzdem, damit sofort deutlich wird, dass es sich um eine Instanzvariable handelt. ;-)