Bitte um Verbesserungsvorschläge

Mane123

Bekanntes Mitglied
Hallo zusammen,

ich habe ein kleines Programm geschrieben, welches ein Fenster erzeugt. Dieses Fenster kann man durch Klicken auf vier Buttons verschieben.


Könnt Ihr bitte den Quelltext mal durschauen, ob da ihr noch was zu Verbessern wisst?

Vielen Dank!

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

import javax.swing.JFrame;

import javax.swing.JButton;
import java.awt.BorderLayout;
import javax.swing.JOptionPane;




public class SchaltflaecheVerschiebenGUI extends JFrame {
	
	//Die Schaltflächen für die Buttons
	private JButton rauf, runter, links, rechts;
	
	//Die Variablen für die Positionierung auf dem Desktop
	private int xachse = 400, yachse = 200;
	
	//Die Variablen, welche die aktuelle Desktopgröße abspeichern
	private int desktopwidth, desktopheight;
	
	//Die Variablen, welche die aktuelle maximal zulässige Verschiebung abspeichern
	private int maxverschiebungunten, maxverschiebungrechts;
	
	//Der Wert für die aktuellen Abmessungen des Fenster
	private int widthFenster = 300, heightFenster = 300;
	
	
	class MeinKompakterListener implements ActionListener {

		@Override
		public void actionPerformed(ActionEvent e) {
			// TODO Auto-generated method stub
			
			if (e.getActionCommand ().equals ("RAUF")) {
				yachse -= 10;
			}
			
			if (e.getActionCommand ().equals ("RUNTER")) {
				yachse += 10;
			}
			
			if (e. getActionCommand (). equals ("LINKS")) {
				xachse -= 10;
			}
			
			if (e.getActionCommand ().equals ("RECHTS")) {
				xachse += 10;
				}
		
	    //Die Methode zur Auswertung der eingegeben X-Achsen und Y-Achsen Werte
		werteAuswerten();
			}			
		}
			
		
		public SchaltflaecheVerschiebenGUI (String titel) {
			//Den Konstruktor der Superklasse aufrufen
			super (titel);
			
			//Die vier Schaltflächen erstellen
			rauf = new JButton ("RAUF");
			runter = new JButton ("RUNTER");
			links = new JButton ("LINKS");
			rechts = new JButton ("RECHTS");
			
			//Das Layout festlegen
			this.setLayout(new BorderLayout(10,10));
			
			add (BorderLayout.NORTH, rauf);
			add (BorderLayout.EAST, rechts);
			add (BorderLayout.SOUTH, runter);
			add (BorderLayout.WEST, links);
		  
		 
			//die Größe des Fensters festlegen
			this.setSize (widthFenster, heightFenster);
		  
		    //die aktuelle Größe des Desktops ermitteln
			//für die Breite:
			
			desktopwidth = FenstergroesseBerechenen.getDesktopwidth ();
			
			//die aktuelle Größe des Desktops ermitteln 
			//für die Höhe:
			
			desktopheight = FenstergroesseBerechenen.getDesktopheight();
			
			//die maximale Verschieben nach unten Berechnen
			
			maxverschiebungunten = desktopheight - heightFenster;
			System.out.println (maxverschiebungunten);
		  
			//die maximale Verschiebung nach rechts Berechnen
			
			maxverschiebungrechts = desktopwidth - widthFenster;
			
		 
			//eine neue Instanz der Listener Klasse erstellen
			MeinKompakterListener listener = new MeinKompakterListener ();
		  
			//die Schaltflächen mit dem Listener verbinden
			rauf.addActionListener (listener);
			runter.addActionListener (listener);
			links.addActionListener (listener);
			rechts.addActionListener(listener);
		  
		
			//die Fenstergröße fixieren
			this.setResizable (false);
		  
			//das Fenster auf dem Desktop plazieren
			this.setLocation(xachse, yachse);
		  
			//die Standardaktion beim Schließen des Fensters festlegen
			setDefaultCloseOperation (JFrame.EXIT_ON_CLOSE);
		  
			//das Fenster anzeigen
			setVisible (true); 		
		}
		
	
		//die Methode, um die geänderten Positions-Koordinaten auszuwerten
		public void werteAuswerten () {
			
			//Prüfen, ob der neue Wert korrekt ist, d. h. auf dem Desktop sichtbar ist
			if (xachse >= 0 && xachse <= maxverschiebungrechts && yachse >= 0 && yachse <= maxverschiebungunten){
				
				setLocation (xachse, yachse);
			}
			
			//falls die Koordinaten ungültig sind, dann erscheint eine Meldung auf dem Desktop
			//und es werden die minimal bzw. maximal zulässigen Werte hinterlegt.
		
			else {
				JOptionPane.showMessageDialog (null, "Ihre Eingabe war ungültig");	
				
				if (xachse < 0)
					xachse = 0;
				
				else
					if (xachse > maxverschiebungrechts)
						xachse = maxverschiebungrechts;
				
				if (yachse < 0)
					yachse = 0;
				
				else 
					if (yachse > maxverschiebungunten)
						yachse = maxverschiebungunten;
				}			
		}
}

und:

Java:
import java.awt.Dimension;
import java.awt.Toolkit;


public class FenstergroesseBerechenen {
	
	
	
	public static int getDesktopwidth () {
		//Variable zur Zwischenspeicherung der Desktopbreite
		int bwidth;
		
		
		Dimension bGroesse =	Toolkit.getDefaultToolkit().getScreenSize();
		
		bwidth = bGroesse.getSize().width;
		return bwidth;	
	}

	
	public static int getDesktopheight () {
		//Variable zur Zwischenspeicherung der Desktophöhe
		int bheight;
		
		Dimension bGroesse =	Toolkit.getDefaultToolkit().getScreenSize();
		
		bheight = bGroesse.getSize().height;
		return bheight;
	}
}


Viele Grüße!
 
getDesktopwidth() + getDesktopheight() sind ja schlimm geraten,
so viele Zeilen, die meisten leer, gar ein Kommentar für eine Variable, dabei passiert doch gar nix,
schreibe
Java:
return Toolkit.getDefaultToolkit().getScreenSize().width;
und fertig ist die Laube

der Rest ist vergleichsweise unauffällig,
- bei if + else besser IMMER Schleifen zu benutzen
- wieso hast du überall Leerzeichen vor den Klammern, bei Methoden-Deklarationen + Aufrufen, normal sieht das nicht aus

public void werteAuswerten() {
statt
public void werteAuswerten () {
usw
 
Hallo,

danke für die Hinweise.

Das mit der Desktopgröße habe ich sofort geändert 🙂 Die eigene Klasse war ja wie Du schon geschrieben hast, sinnlos.

Das mit den Klammern und Leerzeichen ändere ich auch noch ab.

Allerdings habe ich noch eine Frage:

Wie meinst Du das denn mit den if + else Blöcken?

Das sind doch Blöcke?

Oder meinst Du es eher so:

Java:
		public void werteAuswerten () {
			
			//Prüfen, ob der neue Wert korrekt ist, d. h. auf dem Desktop sichtbar ist
				if (xachse >= 0 && xachse <= maxverschiebungrechts && yachse >= 0 && yachse <= maxverschiebungunten){
				setLocation (xachse, yachse); 
				}
			//falls die Koordinaten ungültig sind, dann erscheint eine Meldung auf dem Desktop
			//und es werden die minimal bzw. maximal zulässigen Werte hinterlegt.
				else {
				
					if (xachse < 0) { 
						xachse = 0;
						JOptionPane.showMessageDialog (null, "Ihre Eingabe war ungültig");	
					}
					if (xachse > maxverschiebungrechts) {
						xachse = maxverschiebungrechts;
						JOptionPane.showMessageDialog (null, "Ihre Eingabe war ungültig");	
					}
					if (yachse < 0) {
						yachse = 0;
						JOptionPane.showMessageDialog (null, "Ihre Eingabe war ungültig");	
					}
					if (yachse > maxverschiebungunten) {
						yachse = maxverschiebungunten;
						JOptionPane.showMessageDialog (null, "Ihre Eingabe war ungültig");	
						}								
				}				
		}

Viele Grüße
 
Klammern, wie Ebenius schon korrigiert hat, beim else kannst du ruhig bleiben,
nur
Java:
if {
 ..
} else {
..
}
statt
Java:
if 
  ..
else 
  ..

-----

FenstergroesseBerechenen ist sicherlich ein komischer Klassenname,
aber gegen eine Klasse SwingIrgendwas mit einer Methode getDesktopWidth() hätte ich gar nichtmal was,
ich meinte den Aufbau dieser Methoden,
wenn man sie nur einmal braucht kann man sie natürlich auch ganz weglassen
 
statt
Java:
                    if (xachse < 0) { 
                       // ..
                    }
                    if (xachse > maxverschiebungrechts) {
                        // ..
                    }
                    if (yachse < 0) {
                       // ..
                    }
                    if (yachse > maxverschiebungunten) {
                       // ..
                    }

könnte man

Java:
                    if (xachse < 0) { 
                       // ..
                    }else if (xachse > maxverschiebungrechts) {
                        // ..
                    }
                    
                    if (yachse < 0) {
                       // ..
                    }else if (yachse > maxverschiebungunten) {
                       // ..
                    }

verwenden, denn wenn xachse < 0 braucht er nicht xachse > maxverschiebungrechts prüfen!

da gleiche bei

Java:
            if (e.getActionCommand ().equals ("RAUF")) {
                yachse -= 10;
            }
            
            if (e.getActionCommand ().equals ("RUNTER")) {
                yachse += 10;
            }
            
            if (e. getActionCommand (). equals ("LINKS")) {
                xachse -= 10;
            }
            
            if (e.getActionCommand ().equals ("RECHTS")) {
                xachse += 10;
                }
da könnte man es so machen:

Java:
            String command = e.getActionCommand ();

            if (command .equals ("RAUF")) {
                yachse -= 10;
            }else  if (command .equals ("RUNTER")) {
                yachse += 10;
            }else if (command . equals ("LINKS")) {
                xachse -= 10;
            }else if (command .equals ("RECHTS")) {
                xachse += 10;
            }

Ist natürlich in diesem Fall kein großer Performance gewinn, kann aber in anderen Projekten was bringen wenn da mehrere hundermal druchgegangen wird!
 
Java:
enum Command {
  RAUF(0,-10),RUNTER(0,10),LINKS(-10,0),RECHTS(10,0);
  private int x;
  private int y;
  Command(int x, int y) { this.x = x; this.y = y; }
  int getX() { return x; }
  int getY() { return y; }
}

...

rauf = new JButton (Command.RAUF.toString());
runter = new JButton (Command.RUNTER.toString());
links = new JButton (Command.LINKS.toString());
rechts = new JButton (Command.RECHTS.toString());

...

Command command = Enum.valueOf(Command.class,e.getActionCommand());
xachse += command.getX();
yachse += command.getY();

...
 
Hallo zusammen,

dankeb zuerst mal fuer die Tipps.

@ SlaterB

Also kann ich das mit den if...else so lassen, wie ich es beim ersten mal aufgezeigt habe?

Oder wie ist das denn mit den if...Else Bloecken gemeint?
Ich habe doch auch beim ersten mal Bloecke geschrieben.

Macht das einen Unterschied, ob ich die Klammern weg lasse, oder die Klammern dazu schreibe?

Das Programm hat ohne Klammern auch funktioniert.?

@ Didi_R

Werden bei den Else if nicht auch alle Zweige wie bisher von oben nach unten durchlaufen, oder wird nicht mehr weitergeprueft, sobald z. B. die Anweisung nach RUNTER ausgefuehrt wird?

Viele Gruesse
 
Ich habe doch auch beim ersten mal Bloecke geschrieben.
[..]
Das Programm hat ohne Klammern auch funktioniert.?
die beiden Sätze passen nicht zusammen, es sei denn die definierst Blöcke als etwas anderes als Klammern, z.B. die Einrückung?

ohne Klammern hat man immer die Gefahr
Java:
if (x) 
  y;

auf 

if (x) 
  y;
  z;
zu erweitern und sich zu wundern, warum z; immer ausgeführt wird, die Einrückung ist ganz egal

bei
Java:
if (x) {
  y;
}

zu
if (x) {
  y;
  z;
}
sieht das viel ungefährlicher aus
 
Hallo zusammen,

ich habe eure Ratschläge beherzigt und jetzt müsste das Programm passen. 🙂

Vielen Dank noch einmal.

Ich habe noch einen anderen Quelltext erstellt. Passt dieser soweit, oder habt ihr da auch noch Vorschläge zur Verbesserung?

Vielen Dank!

Java:
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.Font;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.border.BevelBorder;


public class TextSpielereiGUI extends JFrame{
	
	/**
	 * 
	 */
	private static final long serialVersionUID = 5047969757024909032L;
	//die ID wurde automatisch mit Eclipse ergänzt
	

	//ein Label und zwei Schaltflächen als Instanzvariablen
	private JLabel ausgabe, aktuelleSchriftgroesse;
	private JButton schaltflaecheGroesser, schaltflaecheKleiner;
	//für die aktuelle Schriftgröße
	private int schriftGroesse;
	
	//eine innere Klasse für den WindowListener und den ActionListener
	//die Klasse ist von WindowAdapter abgeleitet und
	//implementiert die Schnittstelle ActionListener
	class MeinKompakterListener extends WindowAdapter implements ActionListener{
		//für das Öffnen des Fensters
		@Override
		public void windowOpened(WindowEvent e) {
			//für die Eingabe
			String eingabe;
			eingabe = JOptionPane.showInputDialog("Geben Sie einen Text ein");
			//den Text in das Label setzen
			ausgabe.setText(eingabe);
			//die Schriftgröße beim Start des Programms, vor Veränderungen anzeigen
			aktuelleSchriftgroesse.setText (Integer.toString (schriftGroesse));
		}

		//für die Schaltflächen
		@Override
		public void actionPerformed(ActionEvent e) {
			//wurde auf Größer geklickt
			if (e.getActionCommand() == "<") 
				//die Schriftgröße um 1 erhöhen
				schriftGroesse++;
			//wurde auf Kleiner geklickt
			if (e.getActionCommand() == ">") 
				//die Schriftgröße um 1 verringern
				schriftGroesse--;
			//und neu setzen
			ausgabe.setFont(new Font("Arial", Font.PLAIN, schriftGroesse));
			
			//die aktuelle Schriftgröße anzeigen.
			aktuelleSchriftgroesse.setText (Integer.toString (schriftGroesse));
		}
		
	}

	//der Konstruktor
	//er erzeugt die Komponenten und setzt die Fenstereinstellungen
	public TextSpielereiGUI(String titel) {
		//den Konstruktor der Basisklasse aufrufen und den Fenstertitel übergeben
		super(titel);
		//die beiden Schaltflächen
		schaltflaecheGroesser = new JButton("<");
		schaltflaecheKleiner = new JButton(">");
	
		//die Bildschirmtipps für die Schaltflächen setzen
		schaltflaecheGroesser.setToolTipText("Schrift vergrößern");
		schaltflaecheKleiner.setToolTipText("Schrift verkleinern");
		
		
		//zwei leere Label
		aktuelleSchriftgroesse = new JLabel ();
		ausgabe = new JLabel();
		//die Größe für die Schrift setzen
		schriftGroesse = 10;
		//die Schriftart im Label ausgabe setzen
		ausgabe.setFont(new Font("Arial",Font.PLAIN, schriftGroesse));
		//einen Rahmen um die Label setzen
		
		BevelBorder rahmen = new BevelBorder (BevelBorder.LOWERED);
		
		ausgabe.setBorder(rahmen);
		aktuelleSchriftgroesse.setBorder (rahmen);

			
		//ein Grid Layout anwenden
		setLayout(new GridLayout(0,2,10,10));
		
		add(schaltflaecheGroesser);
		add(schaltflaecheKleiner);
		add (aktuelleSchriftgroesse);
		add(ausgabe);
		
		//den Listener verbinden
		addWindowListener(new MeinKompakterListener());
		schaltflaecheGroesser.addActionListener(new MeinKompakterListener());
		schaltflaecheKleiner.addActionListener(new MeinKompakterListener());
		
		//die Größe des Fensters fest setzen
		//hier auf 600 * 100
		setSize(600, 100);
		//danach aber nicht mehr "packen", sonst wird die Größe direkt wieder verändert
		//pack();
		//die Standardaktion beim Schließen festlegen
		setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
		//automatisch eine gute Position suchen lassen
		//setLocationByPlatform(true);
		//zentriert auf dem Desktop darstellen
		//die Bildschirmauflösung beschaffen und in einer Variablen vom Typ Dimension speichern
		Dimension bGroesse= Toolkit.getDefaultToolkit().getScreenSize();
		//das Fenster positionieren
		setLocation((bGroesse.width - getSize().width) / 2, (bGroesse.height - getSize().height) / 2);
	
		//das Fenster anzeigen
		setVisible(true);
	}
}

Viele Grüße
 
Noch ein paar kleine Anmerkung bezüglich Code-Conventions:

Statt Strings wie RAUF oder RUNTER immer wieder zu benutzen macht es mehr Sinn dafür Konstanten anzulegen.
Java:
public static final String RAUF = "rauf";
// ...
e.getActionCommand ().equals (RAUF)
// und
rauf = new JButton (RAUF);
Hat den Vorteil, dass du dich bei dem Wert von RAUF nicht verschreiben kannst und, dass du den Wert später auch leicht wieder ändern kannst.

Ich würde für Variablennamen immer einen gemeinsamen Nenner suchen. Außerdem sollte man deutsch und englisch nicht mischen. Statt:
Java:
private int widthFenster = 300, heightFenster = 300;
lieber
Java:
private int windowWidth = 300,  windowHeight= 300;
oder alles auf deutsch (ich würde aber englisch empfehlen).

Und hier natürlich auch jedes neue Wort groß schreiben:
Code:
desktopheight
->
Code:
desktopHeight

Aber das sind z.T. auch Ansichtssachen, musst du also nicht unbedingt übernehmen. 🙂
 
Java:
public static final String RAUF = "rauf";
Immer noch besser als überall mit dem String zu hantieren, aber eigentlich sollte man solche Konstanten durch enums ersetzen. Vorteil ist, dass man in diese Funktionalität einbauen kann (z.B. die Bewegungsrichtung, oder wenn man das Programm später mal internationalisieren will).
 
Vorsicht, die Werte werden sowohl für das ActionCommand als auch für den Text eines Buttons verwendet. Eigentlich sollte man beide nichtmal in der selben Konstante zusammenfassen. Nur dass zwei String (auch wenn sie derzeit Literale sind) den selben Wert haben, heißt das noch lange nicht, dass man sie im Code zusammenfassen will.

Landei, Enums für ActionCommands halte ich für theoretisch möglich, praktisch aber für ungünstig, weil die Swing-API nunmal Strings haben möchte. Enums für in UIs angezeigte Strings halte ich hingegen einfach so für einen Fehler. ;-)

Ebenius
 
Landei, Enums für ActionCommands halte ich für theoretisch möglich, praktisch aber für ungünstig, weil die Swing-API nunmal Strings haben möchte. Enums für in UIs angezeigte Strings halte ich hingegen einfach so für einen Fehler. ;-)

Ebenius
In diesem Fall sind rauf, runter u.s.w. mehr als nur String-Konstanten, sie folgen alle einer gewissen Logik (Bewegung in einer bestimmten Richtung). Eine eigene Klasse wäre hier wahrscheinlich Overkill, aber enums halte ich noch für vertretbar. Wahrscheinlich ist es nicht ganz "sauber", einfach enum.toString() zu verwenden, eine separate Methode wäre wohl besser (auch wenn sie erst mal zu toString() delegiert).
 
Enums für in UIs angezeigte Strings halte ich hingegen einfach so für einen Fehler. ;-)
meinst du damit Austauschbarkeit durch andere Sprachen/ irgendwann Wechsel der Vorlieben in der GUI ohne das restliche Programm zu berühren?

dann kann man eine Indirektion einführen, die aber wenigstens noch durch die Enum gesteuert werden sollte
GUITexter.getCurrentText(enumWert,language);
darin switch oder so
 

Neue Themen


Zurück
Oben