Code in Hinsicht auf Lesbarkeit optimieren

[JavaMeetsBlueJ]

Hallo Java-Forum,
der folgende Code beschschreibt ein simples Programm und ist meiner Meinung nach pinibel kommentiert. Mich interessiert einfach, was da noch fehlt, bzw. was ich da noch besser machen kann.
MfG

Klasse Main:

public class Main {

	
	public static void main(String[] args) {
		GUI fenster = new GUI();             // ein Fenster wird erzeugt
		

	}

}

Klasse GUI:

import java.awt.event.ActionListener;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;


public class GUI extends JFrame{
	
	// Variablen anlegen und Objekte der Steuerelemente erzeugen
	
	JPanel panel1 = new JPanel();
	JButton addiereButton = new JButton("Addieren");
	JButton subtrahiereButton = new JButton("Abziehen");
	JButton button3 = new JButton("Sperren");
	JTextField txtfield = new JTextField("0",20);
	boolean locked = false;
	
public GUI()
{
	// Fensterattribut festlegen
	setSize(500,100);
	setResizable(false);
	setLocationRelativeTo(null);
	setTitle("Titel");
	setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
	
	//-----------------------------ActionListener erzeugen----------------------------------------
	
	ActionListener al = new ActionHandler(this);
	
	//---------------------------------Steuerelemente dem Fenster hinzufügen-------------------------------------
	add(panel1);
	panel1.add(addiereButton);
	panel1.add(subtrahiereButton);
	panel1.add(button3);
	
	addiereButton.addActionListener(al);
	subtrahiereButton.addActionListener(al);
	button3.addActionListener(al);
	
	panel1.add(txtfield);
	//-------------------------------------Fenster sichtbar machen---------------------
	setVisible(true);
}

/**
 * ändert den Inhalt des Textfelds txtfield
 * @param zahl Zahl, die in dem Textfeld steht
 */
public void setTextFieldText(int zahl) 
{
    txtfield.setText("" + zahl);	
}

/**
 * verhindert das Editieren von txtfield
 */
public void sperreTextfield()
{
	txtfield.setEditable(false);
	locked = true;
	button3.setText("Entsperren");
}

/**
 * ermöglicht das Editieren von txtfield
 */
public void entsperreTextfield()
{
	txtfield.setEditable(true);
	locked = false;
	button3.setText("Sperren");
}

}

Klasse ActionHandler:

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;


public class ActionHandler implements ActionListener {

	 GUI fenster;
	 int zahl;
	 
	 
	public ActionHandler(GUI fenster)
	{
		this.fenster = fenster;                             // Übergabe der Referenz des Fensters
	}
	
	@Override
	public void actionPerformed(ActionEvent e) {
		
		// falls addiereButton geklickt wurde
		if (e.getSource().equals(fenster.addiereButton)) 
		{
			zahl++;
			fenster.setTextFieldText(zahl);
		}
		
		// falls subtrahiereButton geklickt wurde
		else if (e.getSource().equals(fenster.subtrahiereButton))
			{
			zahl--;
			fenster.setTextFieldText(zahl);
			}
		
		// falls buuton3 geklickt wurde
		else if(e.getSource().equals(fenster.button3))
		{
			if(fenster.locked == false)  // wenn txtfield editierbar
			{
				fenster.sperreTextfield();
			}
			else if(fenster.locked)        // wenn txtfield nicht editierbar
			{
				fenster.entsperreTextfield();
			}
		}
		

		
	}

}

 

[Constructor]

Du könntest z.B. mal Conventions und Clean-Code in betracht ziehen, denn in deiner main ist schon ein großer Fehler : deine GUI erbt von JFrame (schon schlecht genug für sich alleine), also hat das setVisible() NICHTS in dessen Konstruktor zu suchen. Stattdessen sollte deine main eher so aussehen :

GUI gui=new GUI();
gui.setVisible(true);

Ist an sich zwar genau so Käse weil wozu von JFrame erben aber im Vergleich zu setVisible im Konstrukor callen immer noch besser.

 

[Fab1]

Instanzvariablen sollten private sein. Diese werden dann mit Getter und Setter Methoden geändert.

 

[JavaMeetsBlueJ]

Du könntest z.B. mal Conventions und Clean-Code in betracht ziehen, denn in deiner main ist schon ein großer Fehler : deine GUI erbt von JFrame (schon schlecht genug für sich alleine), also hat das setVisible() NICHTS in dessen Konstruktor zu suchen. Stattdessen sollte deine main eher so aussehen :

GUI gui=new GUI();
gui.setVisible(true);

Ist an sich zwar genau so Käse weil wozu von JFrame erben aber im Vergleich zu setVisible im Konstrukor callen immer noch besser.

- Wieso sollte GUI nicht von JFrame erben?
-Gilt das nur für setVisible() oder auch für alle anderen Methoden genauso?

Bin halt noch nicht so lange dabei und freue mich da über jedes Feedback
MfG

 

[Gast2]

- Wieso sollte GUI nicht von JFrame erben?

Weil du JFrame nicht um innovativ neue Funktionalität erweiterst
Es reicht völlig aus wenn GUI eine Instanz von JFrame verwaltet.

-Gilt das nur für setVisible() oder auch für alle anderen Methoden genauso?

In dem Fall nur für setVisible(). Der Konstruktor ist dafür zuständig das Objekt zusammenzubauen. Das Anzeigen gehört da nicht unbedingt mit dazu.

 

[JavaMeetsBlueJ]

Ok das mit dem JFrame ist mir jetzt auch klar, nur wie sieht das jetzt aus mit den Buttons, sollen die auch private sein, weil der ActionListener dann nicht auf sie zugreifen kann/darf... es ist doch total umständlich für jeden Button ne Getter-Methode zu machen.
Beispiel:

public JButton gibMirButton1()
{
return button1;
}

MfG

 

[Gast2]

Das ist aber der gängige Weg

Ich würde deinen ActionListener aber eher als anonyme Klassen realisieren, vor allem weil das ja nur je zwei Zeilen Code sind die du da ausführen willst.

 

[JavaMeetsBlueJ]

Ja das mit anonymen Klasse wieß ich, nur ich versteh noch nicht so ganz diese Art von Klassen und bleibe gerade eher dabei, den ActionListener zu implementieren, weil ich deisen Weg zu 100% nachvollziehen kann und ungerne Sachen code, wo ich weiß es geht, aber ohne zu wissen wie... .

Schade, dass das mit dme Button so umständlich gehalten wird, aber COnvention sind Conventions und je eher man sich dran gewöhnt, desto besser

Kann man sich hieran ruhig orientieren?

Einstieg in Java. Das Praxis-Training - Das Video-Training von Galileo Computing

MfG

 

[tommysenf]

Hi,

hier ein paar eigene Erfahrungen aus großen Softwareprojekten.

1. Das A und O für verständlichen Code sind meiner Meinung nach gute Variablen und Methodennamen.

    JPanel panel1 = new JPanel();
    JButton addiereButton = new JButton("Addieren");
    JButton subtrahiereButton = new JButton("Abziehen");
    JButton button3 = new JButton("Sperren");
    JTextField txtfield = new JTextField("0",20);

addiereButton und subrahiereButton sind sind gut gewählt, wer den Variablennamen liest weiss sofort, was gemacht wird. panel1, button3 und txtfield sind verbesserungswürdig:

    JPanel grundPanel = new JPanel();
    JButton addiereButton = new JButton("Addieren");
    JButton subtrahiereButton = new JButton("Abziehen");
    JButton sperrButton = new JButton("Sperren");
    JTextField ergebnisTextField = new JTextField("0",20);

2. Sichtbarkeit

Das GUI Elemente nicht private gemacht werden, sonder package deklariert sind, ist meiner Erfahrung nach gängige Praxis die ich schon oft erlebt habe und würde von mir toleriert werden, da es den GUI code meines Erachtens auch kompakter und lesbarer macht.

locked ist jedoch eine Eigenschaft und daher definitiv auf private zu setzen. Also:

public class GUI extends JFrame {
    ...
    private boolean locked = false;
    ...

    public void setLocked(boolean state) {
       this.locked = state;
    }

    public boolean isLocked() {
       return locked;
    }

3. Lesbarer Code

Guter Code liest sich wie natürliche englische Sprache und sollte auch so geschrieben werden.
Beispiel:

Allein, durch die Einführung des getters isLocked() können wir aus

// falls buuton3 geklickt wurde
        else if(e.getSource().equals(fenster.button3))
        {
            if(fenster.locked == false)  // wenn txtfield editierbar
            {
                fenster.sperreTextfield();
            }
            else if(fenster.locked)        // wenn txtfield nicht editierbar
            {
                fenster.entsperreTextfield();
            }
        }

folgendes machen:

         // falls buuton3 geklickt wurde
        else if(e.getSource().equals(fenster.button3)) {
            if(fenster.isLocked()) {  // wenn txtfield nicht editierbar
                fenster.entsperreTextfield();
            } else {
                fenster.sperreTextfield();
            }
        }

4. Kommentare

Inline-Kommentare sollten

1. nicht redundant sein
2. so wenig wie möglich nötig sein

Ein Kommentar ist meist ein Zeichen für schlecht formulierten Code oder oft auch einfach überflüssig:

ActionListener al = new ActionHandler(this); //ActionListener erzeugen
if(fenster.isLocked()) {  // wenn txtfield nicht editierbar

Sind meiner Meinung nach redundant und können ersatzlos gestrichen werden. Das Problem mit diesen Kommentaren ist, das sie bei Codeänderungen oft nicht gepflegt werden und irgendwann ist der Kommentar dann eventuell sogar falsch,was viel schlechter ist als gar kein Kommentar.

Andere Kommentare lassen sich über Methoden einpflegen. Das macht den Code Lesbar und der "Kommentar" wird dann auch mitgewartet:

        else if(sperrButtonClicked(e)) {
            if(fenster.isLocked()) { 
                fenster.entsperreTextfield();
            } else {
                fenster.sperreTextfield();
            }
        }
...

       private boolean sperrButtonClicked(ActionEvent click) {
          return klick.getSource().equals(fenster.sperrButton)
       }

Auch Kommentare die eine Gliederung darstellen, zeigen meist nur, dass man das Ganze besser in extra Methoden auslagern sollte:

]public GUI()
{
// Fensterattribut festlegen
    setSize(500,100);
    setResizable(false);
    setLocationRelativeTo(null);
    setTitle("Titel");
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    
    //-----------------------------ActionListener erzeugen----------------------------------------
    
    ActionListener al = new ActionHandler(this);
    
    //---------------------------------Steuerelemente dem Fenster hinzufügen-------------------------------------
    add(panel1);
    panel1.add(addiereButton);
    panel1.add(subtrahiereButton);
    panel1.add(button3);
    
    addiereButton.addActionListener(al);
    subtrahiereButton.addActionListener(al);
    button3.addActionListener(al);
...
}

Wird zu:

public GUI() {
  initializeWindow();
  ActionListener actionListener = new ActionHandler(this);
  initalizeActions(actionListener);
}

private void initalizeWindow() {
    setSize(500,100);
    setResizable(false);
    setLocationRelativeTo(null);
    setTitle("Titel");
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}
...

Ansonsten würde ich dir noch raten, dir gerade bei Gui Anwendungen einmal das MVC-Pattern anzuschauen, um ein besseres Design deiner Klassen zu erreichen. Weiterhin bevorzuge ich englische Bezeichner, da Java und die Klassenbibliothek der englischen Sprache entstammen und der Code meines erachtens dann besser lesbar ist als in "Denglisch".

 

[gatsantwort]

Das ist aber der gängige Weg

Schade, dass das mit dme Button so umständlich gehalten wird, aber COnvention sind Conventions und je eher man sich dran gewöhnt, desto besser

Nein, das ist NICHT der gängige Weg! Der gängige Weg ist deine Instanzvariablen private zu machen und keine getter und setter Methoden zur Verfügung zu stellen. Gerade bei GUIs ist es mir völlig unverständlich warum es eine Methode getXYZButton geben sollte...

 

[JavaMeetsBlueJ]

Danke, ich hatte nämlich gestern garkeinen Bock mehr weiterzumachen, weil ich mich langsam an Conventions gewöhnen möchte und es mir echt sehr umständlich ershcien, jetzt hab ich wieder Mut
Ja das mit dem MVC hab ich mir auch noch vorgenommen.... evtl werde ich nachher noch den code meines BMI-Rechners posten, an dem ich gerade sitze, wenn mir das jemand an dem Beispiel veranschaulichen könnte,wäre das super, falls nicht, muss ich halt einfach nachlesen. Danke

MfG

 

[MVC-rate]

Ich gebe noch zu bedenken dass das MVC-Pattern lediglich EIN mögliches Pattern ist wie man sein Programm strukturieren kann. Es ist jetzt aber auch nicht "DAS" Pattern, also nicht die Lösung auf alles. Bei GUIs macht es sich aber meist ganz gut.

 

[JavaMeetsBlueJ]

Also, ich sitze gerade an meinem BMI-Rechner, um alles zu üben, was ich bisher kann, das Problem ist nur folgendes:

Wenn ich die Methode, die ich für die Berechnung brauche, in den ActionListener mit rein tu, ist das von Design her absolut schlecht, aber alles funktioniert.

Mein Ziel ist es, die Methode in eine Extra Klasse "Berechnungen" zu packen, wo die Logik drin ist.

Hier tun sich folgende Probleme auf: Ich kann an eine Klasse selber keine Referenzen auf andere Objekte speichern, zumindest weiß ich nicht wie das geht-> ich bekomme eine NullPointerException, wenn ich die Methoden in der Klasse Berechnungen ausführe, die auf Methoden des Fensters zugreifen.

Wenn ich jetzt aus der Klasse Berechnungen ein Objekt erzeuge, und danach eines vom ActionListener, bekomm ich auch eine NullpointerException, was ich allerdings überhaupt nicht verstehe, da im GUI-Konstruktor zuerst ein Berechnungen-Objetkt und danach ein ActionListener erzeugt wird.

Hat da jemand einen Tip für mich, wie ich das lösen könnte?

(Ja ich weiß, dass der Post nicht mehr zur Kathegorie gehört, aber ich weiß nicht wohin damit. ggf. einfach verschieben *sorry*)
[EDIT]Das Problem hab ich von selbst gelöst(geiles Feeling) Back to Topic[/EDIT]
MfG