Einfacher Taschenrechner Verbesserungsvorschläge

[Chucky]

Moin,
hab gerad mal als Übungsaufgabe nen einfachen Rechner programmiert.
Der funktioniert zwar aber was die Ereignisbehandlung angeht denke ich es is nen bisschen üppig.
Der Rechner rechnet nur ganzzahlig und besitzt auch nur die Grundrechenarten "+, -, *, /"
Wäre nett wenn ihr irgendwelche Verbesserungsvorschläge habt.
Bin mir auch nie so ganz sicher wo ich jetzt eine Unterteilung in eine andere Klasse machen soll
wie ihr seht steht der ganze Code bei mir in einer Klasse.
Also schluss mit dem Gelaber hier der Code:
(Danke schonmal für die Hilfe)

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class Rechner_Frame extends JFrame implements ActionListener{
	
	JButton but1;
	JButton but2;
	JButton but3;
	JButton but4;
	JButton but5;
	JButton but6;
	JButton but7;
	JButton but8;
	JButton but9;
	JButton but0;
	
	JButton addition;
	JButton subtraktion;
	JButton division;
	JButton multiplikation;
	JButton gleich;
	
	JPanel numpan;
	
	JTextField display;
	String displayString;
	
	char operator;
	int operand;

	public Rechner_Frame(){
		
		getContentPane().setLayout(new BorderLayout());
		setDefaultCloseOperation(EXIT_ON_CLOSE);
		
		//Einrichten der Zahlen-Buttons
		but1=new JButton("1");
		but2=new JButton("2");
		but3=new JButton("3");
		but4=new JButton("4");
		but5=new JButton("5");
		but6=new JButton("6");
		but7=new JButton("7");
		but8=new JButton("8");
		but9=new JButton("9");
		but0=new JButton("0");
		
		addition=new JButton("+");
		subtraktion=new JButton("-");
		division=new JButton("/");
		multiplikation= new JButton("*");
		gleich=new JButton("=");
		
		//Einrichten des Displays
		displayString=("0");
		display=new JTextField(displayString);
		
		//Einrichten des Zahlen-Panels
		numpan=new JPanel();
		numpan.setLayout(new GridLayout(5,3));
		numpan.add(but7);
		numpan.add(but8);
		numpan.add(but9);
		numpan.add(but4);
		numpan.add(but5);
		numpan.add(but6);
		numpan.add(but1);
		numpan.add(but2);
		numpan.add(but3);
		numpan.add(but0);
		numpan.add(addition);
		numpan.add(subtraktion);
		numpan.add(division);
		numpan.add(multiplikation);
		numpan.add(gleich);
				
		getContentPane().add(display, BorderLayout.NORTH);
		getContentPane().add(numpan, BorderLayout.CENTER);
		
		//Ereignisbehandlung der Buttons
		but1.addActionListener(this);
		but2.addActionListener(this);
		but3.addActionListener(this);
		but4.addActionListener(this);
		but5.addActionListener(this);
		but6.addActionListener(this);
		but7.addActionListener(this);
		but8.addActionListener(this);
		but9.addActionListener(this);
		but0.addActionListener(this);
		addition.addActionListener(this);
		subtraktion.addActionListener(this);
		division.addActionListener(this);
		multiplikation.addActionListener(this);
		gleich.addActionListener(this);
				
	}
	
	public void actionPerformed(ActionEvent e){
		if(display.getText().equals("0")){
			displayString="";
			display.setText(displayString);
		}
		
		//Implementierung der Ereignisabfrage für die Zahleingabe
		if(e.getSource()==but1||
		   e.getSource()==but2||
		   e.getSource()==but3||
		   e.getSource()==but4||
		   e.getSource()==but5||
		   e.getSource()==but6||
		   e.getSource()==but7||
		   e.getSource()==but8||
	       e.getSource()==but9||
	       e.getSource()==but0){
			displayString=displayString+e.getActionCommand();
			display.setText(displayString);
		}
		
		if(e.getSource()==addition||
		   e.getSource()==subtraktion||
		   e.getSource()==multiplikation||
		   e.getSource()==division||
		   e.getSource()==gleich){
			switch(operator){		
				case'+': displayString=(String.valueOf(operand + Integer.parseInt(display.getText())));
						 display.setText(displayString);
						 break;
				case'-': displayString=(String.valueOf(operand - Integer.parseInt(display.getText())));
				    	 display.setText(displayString);
					     break;
			    case'*': displayString=(String.valueOf(operand * Integer.parseInt(display.getText())));
				    	 display.setText(displayString);
					     break;
			    case'/': displayString=(String.valueOf(operand / Integer.parseInt(display.getText())));
				    	 display.setText(displayString);
					     break;
			}	
		}
		
		if(e.getSource()==addition){
			operand=Integer.parseInt(display.getText());
			operator='+';
			displayString="";
		}
		
		if(e.getSource()==subtraktion){
			operand=Integer.parseInt(display.getText());
			operator='-';
			displayString="";
		}
		
		if(e.getSource()==division){
			operand=Integer.parseInt(display.getText());
			operator='/';
			displayString="";
		}
		
		if(e.getSource()==multiplikation){
			operand=Integer.parseInt(display.getText());
			operator='*';
			displayString="";
		}
		
		if(e.getSource()==gleich){
			if(operator=='+'||operator=='-'||operator=='/'||operator=='*'){
					operator='0';
			}
		}
		
	}
	
}

 

[mic_checker]

Nur nach schnellem Überfliegen von dem Code:
Dadurch das du ActionListener implementierst wirkt actionPerformed etwas unübersichtlich, ich würde zumindest separate ActionListener für die Berechnungen (+,-,etc.) und die Darstellung der Zahlen (1,2,...) anbieten.

Zudem solltest du noch auf NumberFormatException aufpassen.

 

[Bleiglanz]

bissl verbessern

JButton but1; // blöder name, warum nicht button1
   
JButton addition; // besser additionButton

//Einrichten der Zahlen-Buttons
      but1=new JButton("1");
      but2=new JButton("2");
      but3=new JButton("3");
      but4=new JButton("4");
      but5=new JButton("5");
      but6=new JButton("6");
      but7=new JButton("7");
      but8=new JButton("8");
      but9=new JButton("9");
      but0=new JButton("0"); // lieber eine schleife,

// oder gleich ein array[0...9] für die Tasten, ist doch besser....

    
//Implementierung der Ereignisabfrage für die Zahleingabe

// sowas hier ist ganz schlecht...

      if(e.getSource()==but1||
         e.getSource()==but2||
         e.getSource()==but3||
         e.getSource()==but4||
         e.getSource()==but5||
         e.getSource()==but6||
         e.getSource()==but7||
         e.getSource()==but8||
          e.getSource()==but9||
          e.getSource()==but0){

würde dir empfehlen (auch um ein bissl OO zu lernen), die Operationen + - * / als Klassen zu realisieren und via Polymorphie dann die ganzen if's einzusparen

UND:

trenne den Code für die Anzeige von dem für den Rechner, versuch mal ein "Rechenmodul" zu schreiben, das man ggf. auch über die Tastatur bedienen könnte

 

[Chucky]

hmm ok,
aber wenn ich die Rechenarten in eine eigene Klasse schreibe wären
diese Klassen doch nur eine Ansammlung von statischen Methoden oder?

Mfg Chucky

 

[Sky]

hmm ok,
aber wenn ich die Rechenarten in eine eigene Klasse schreibe wären
diese Klassen doch nur eine Ansammlung von statischen Methoden oder?

Mfg Chucky

Wieso nur statische Methoden?

 

[Chucky]

Warum sollte man denn ein Objekt aus Plus oder Minus erzeugen?
Man braucht doch nur die Rechenalgorithmen aus den Klassen oder?

Mfg Chucky

 

[Bleiglanz]

nein, dachte mehr an

class Plus implements Operation

class Minus implements Operation

...
(oder auch mit extends von einer Oberklasse)

=> dann erzeugst du jeweils genau eine Instanz

p = new Plus()
m = new Minus()
...
und arbeitest wo immer möglich mit der Oberklasse/dem Interface

static würde nichts bringen, weil du dann trotzdem alles "ausschreiben" musst

 

[Chucky]

war polymorphie das mit konstruktoren überladen???
und schnittstellen versuch ich nochmal zu verstehen, aber das war eines der sachen
bei denen ich erhebliche probleme hatte

Mfg chucky

 

[Chucky]

So ich habe fertig:

public class Rechner_Frame extends JFrame{
	
	JButton[] zahlenButtons;
	JButton gleichButton;
	JPanel numpan;
	JTextField display;
	String displayString;
	ArrayList<Operation> list=new ArrayList<Operation>();
	Operation lastOperation;
	int operand;
	
	public Rechner_Frame(){
		
		//Liste wird mit Operationen gefüllt
		list.add(new Addition());
		list.add(new Subtraktion());
		list.add(new Multiplikation());
		list.add(new Division());
		
		getContentPane().setLayout(new BorderLayout());
		setDefaultCloseOperation(EXIT_ON_CLOSE);
		
		//Einrichten der Zahlen-Buttons
		zahlenButtons=new JButton[10];
		for(int i=0; i<10; i++){
			zahlenButtons[i]=new JButton(String.valueOf(i));
			zahlenButtons[i].addActionListener(new ZahlenListener());
			}
		
		gleichButton=new JButton("=");
		
		//Einrichten des Displays
		displayString=("0");
		display=new JTextField(displayString);
		display.setHorizontalAlignment(SwingConstants.RIGHT);
		
		//Einrichten des Zahlen-Panels
		numpan=new JPanel();
		numpan.setLayout(new GridLayout(5,3));
		numpan.add(zahlenButtons[7]);
		numpan.add(zahlenButtons[8]);
		numpan.add(zahlenButtons[9]);
		numpan.add(zahlenButtons[4]);
		numpan.add(zahlenButtons[5]);
		numpan.add(zahlenButtons[6]);
		numpan.add(zahlenButtons[1]);
		numpan.add(zahlenButtons[2]);
		numpan.add(zahlenButtons[3]);
		numpan.add(zahlenButtons[0]);
		//Schleife um Buttons zu adden und mit einem ActionListener zu versehen
		for(int i=0; i<list.size(); i++){
			JButton button=new JButton(list.get(i).getSymbol());
			numpan.add(button);
			button.addActionListener(new OperationListener(list.get(i)));
		}
		numpan.add(gleichButton);
		
		gleichButton.addActionListener(new GleichListener());
				
		getContentPane().add(display, BorderLayout.NORTH);
		getContentPane().add(numpan, BorderLayout.CENTER);
				
	}
	
	class OperationListener implements ActionListener{
		
		Operation operation;
	
		public OperationListener(Operation o){
			operation=o;
		}
	
		public void actionPerformed(ActionEvent e){
			if(lastOperation!=null){
				operand=lastOperation.work(operand,Integer.parseInt(display.getText()));
				lastOperation=operation;
			}else{
				operand=Integer.parseInt(display.getText());
				lastOperation=operation;
			}
			display.setText(String.valueOf(operand));
			displayString="";
		}
	}
	
	class ZahlenListener implements ActionListener{
	
		public void actionPerformed(ActionEvent e){
			if(displayString.equals("0")){
				displayString=e.getActionCommand();
			}else
				displayString+=e.getActionCommand();
			display.setText(displayString);
		}
	}
	
	class GleichListener implements ActionListener{
		
		public void actionPerformed(ActionEvent e){
			if(lastOperation!=null){
				operand=lastOperation.work(operand,Integer.parseInt(display.getText()));
				display.setText(String.valueOf(operand));
				lastOperation=null;
				}
		}
	}
}

Das Interface Operation:

public interface Operation {
	int work(int x, int y);
	String getSymbol();
}

Beispiel einer Operatorklasse:

public class Multiplikation implements Operation{
	
	public int work(int faktor1, int faktor2){
		
		int produkt;
		produkt=faktor1*faktor2;
		return produkt;
		
	}
	
	public String getSymbol(){
		
		return "x";
	
	}

}

Kritik ist erwünscht Lob aber auch :lol:

 

[bygones]

hat zwar damit nicht viel zu tu - mir war aber langweilig und ich wollte die Enums noch ausprobieren:

interface Operation {
	public double calc(double x, double y);
}

enum Calc implements Operation {
	PLUS {
		public double calc(double x, double y) {
			return x + y;
		}
	},
	MINUS {
		public double calc(double x, double y) {
			return x - y;
		}
	},
	DIVIDE {
		public double calc(double x, double y) {
			return x / y;
		}
	},
	MULTIPLY {
		public double calc(double x, double y) {
			return x * y;
		}
	}
}

public class TestCalc {
	public static void main(String[] args) {
		double x = 3;
		double y = 2;

		for ( Calc c : Calc.values() ) {
			System.out.println( c.calc( x, y ) );
		}
	}
}

 

[Chase]

Kritik ist erwünscht Lob aber auch

Dann mal beides, von Anfänger zu Anfänger:
Das Programm sieht net schlecht aus, auch gut struckturiert etc. Die zweite Version ist auch objektorientiert, woran ich mir momentan die Zähne ausbeisse.
[edit]Ich hab die gleiche Frage in nem eigenen Thread gestellt, aber warum muessen die Operatoren eigentlich muehevoll erzeugt werden, ein Interface impementieren und in ein Array gepackt werden ? Wie du sagtest, statisch waer das doch viel einfacher..???:L[/edit]
Allerdings machen es dir die Events auch ziemlich leicht, daher bleibt die Logik etwas auf der Strecke.
Ich hab ebenfalls mein erstes Programm geschrieben, auch ein einfacher Taschenrechner. Nur: Das ganze in Konsolenform. Daher hast du mit ner Menge Problemen zu kaempfen, wie etwa Stringanalyse, Rekursion, und recht vielen Abfragen. Es war aber ne recht gute Übung für mich, also wenn du nicht zwingend GUI-Programmierung machen willst, versuch das mal

 

[Beni]

So, hab dir noch ein Review versprochen.

Grundsätzlich finde ich das Programm gut, die Mängelliste ist eher kosmetischer Natur :wink:
Nur um eines würde ich dich bitten: entweder englische oder deutsche Namen, aber nicht mischen.

public class Rechner_Frame extends JFrame{

Rencher_Frame... seltsamer Name. Wenn schon "RechnerFrame". Wieso? Wegen den Codingconventions von Sun, an die sich eigentlich jeder Java-Programmierer (Professoren und Lehrer ausgenommen) hält.
zum angucken.

	JButton[] zahlenButtons;
	JButton gleichButton;
	JPanel numpan;
	JTextField display;
	String displayString;
	ArrayList<Operation> list=new ArrayList<Operation>();
	Operation lastOperation;
	int operand;

Interessiert es ausser dem Rechner_Frame sonst noch jemanden, was da für Variablen rumschwirren? Ich denke nicht, mach die Variablen private. Falls doch jemand eine der Variablen benötigt, kannst du immernoch ein Getter machen (z.B. "public int getOperand(){ return operand; }")

	ArrayList<Operation> list=new ArrayList<Operation>();

Es ist dir doch später egal, ob das eine ArrayList, ein Vector oder eine LinkedList ist.
Du kannst auch einfach hinschreiben:

	private List<Operation> list=new ArrayList<Operation>();

JPanel numpan;

Hey, du programmierst Java, nicht C. Es herrscht also kein Bedürfnis möglichst kryptische Namen zu wählen.
Die paar zusätzlichen Buchstaben die du für "numberPanel" benötigst werden dir später helfen dein eigenes Programm wieder zu verstehen...

		displayString=("0");

Hrms, "Faulheit ist guuut"

		displayString="0";

		numpan.add(zahlenButtons[7]);
		numpan.add(zahlenButtons[8]);
		numpan.add(zahlenButtons[9]);
		numpan.add(zahlenButtons[4]);
		numpan.add(zahlenButtons[5]);
		numpan.add(zahlenButtons[6]);
		numpan.add(zahlenButtons[1]);
		numpan.add(zahlenButtons[2]);
		numpan.add(zahlenButtons[3]);
		numpan.add(zahlenButtons[0]);

Naja, das schreit doch geradezu nach einer Schleife!

for( int i = zahlenButtons.length-1; i>=0; i-- )
  numpan.add( zahlenButtons[i] );

		for(int i=0; i<list.size(); i++){

Eine nette Variante ist auch:

          for( Operation op : list ){

	class OperationListener implements ActionListener{
		public void actionPerformed(ActionEvent e){
			display.setText(String.valueOf(operand));
			displayString="";
		}
	}

Dieser beiden Zeilen die mit "display..." beginnen: berfekte Kandidaten für eine eigene Methode.

	class ZahlenListener implements ActionListener{
	
		public void actionPerformed(ActionEvent e){
			if(displayString.equals("0")){
				displayString=e.getActionCommand();
			}else
				displayString+=e.getActionCommand();
			display.setText(displayString);
		}
	}

Da würde ich dasselbe wie beim OperationListener machen. Also: eine Variable in der die Zahl gespeichert ist, und das ActionCommand vergessen.

Aus dem Gleichlistener:

operand=lastOperation.work(operand,Integer.parseInt(display.getText()));

Und wenn der Benutzer was seltsames in Display eingegeben hat?
Mach da noch einen try-catch-Block, der NumberFormatException's abfängt.

public interface Operation {
	int work(int x, int y);
	String getSymbol();
}

Die Methoden sind nur im aktuellen Package sichtbar, aber eigentlich könnte jedermann eine neue Operation implementieren.
Daher:

public interface Operation {
	public int work(int x, int y);
	public String getSymbol();
}

Gewöhn dir das mit den Packageinternen Variablen/Methoden gar nicht erst an (verursacht mehr Probleme als es löst). Mach immer ein public, protected oder private vor die Variablen/Methoden.

Dann bleiben noch:

	JButton[] zahlenButtons;
	JButton gleichButton;
	JPanel numpan;

Die Variablen benötigst du gar nie. Klar, im Konstruktor benutzt du sie um die Buttons auf das Fenster zu kriegen, aber nur im Konstruktor. Keine andere Methode greifft auf diese Variablen zu. Also: weg damit.

 

[Chucky]

Moin Beni!
Vorweg erstamal danke für das recht umfangreiche Review.
Wie du dir sicherlich denkst habe ich jedoch noch einige Rückfragen

Naja, das schreit doch geradezu nach einer Schleife!
Code:

1
2

for( int i = zahlenButtons.length-1; i>=0; i-- )
  numpan.add( zahlenButtons[i] );

Das wollte ich nicht mit einer Schleife machen um dieses typische Taschenrechnerlayout beizubehalten:

789
456
126
0

Eine nette Variante ist auch:

for( Operation op : list ){

Wie kann ich denn bei dieser Variante die Listenelemente einzeln ansprechen?

Die Methoden sind nur im aktuellen Package sichtbar, aber eigentlich könnte jedermann eine neue Operation implementieren.

In meinem Buch steht, dass Schnittstellen und deren Elemente automatisch als public gelten.

Die Variablen benötigst du gar nie. Klar, im Konstruktor benutzt du sie um die Buttons auf das Fenster zu kriegen, aber nur im Konstruktor. Keine andere Methode greifft auf diese Variablen zu. Also: weg damit.

Als lokale Variablen im Konstruktor einführn oder wie meinst das?

Vielen Dank Mfg Chucky

 

[Beni]

Naja, das schreit doch geradezu nach einer Schleife!
Code:

1
2

for( int i = zahlenButtons.length-1; i>=0; i-- )
  numpan.add( zahlenButtons[i] );

Das wollte ich nicht mit einer Schleife machen um dieses typische Taschenrechnerlayout beizubehalten:

789
456
123
0

Ah, ich hab übersehen, dass die Reihenfolge der Indices nicht so "intuitiv" ist :wink:

Ok, aber mit einer Schleife könntest du immernoch Code sparen:

for( int i = 2; i >= 0; i-- ){
  for( int j = 1; j <= 3; j++ )
    numpan.add( zahlenButtons[ 2*i + j ] );
}
numpan.add( zahlenButtons[0] );

Eine nette Variante ist auch:

for( Operation op : list ){

Wie kann ich denn bei dieser Variante die Listenelemente einzeln ansprechen?

"op" wird für jeden Durchgang auf das nächste Listeneelement gesetzt.
also:

for( Operation op : list ){
  ... // hier ist op jedesmal das aktuelle Element
}

ist (vom Verhalten her) identisch mit

for( int i = 0; i < list.size(); i++ ){
  Operation op = list.get( i );
  ...// hier ist op jedesmal das aktuelle Element
}

Die Methoden sind nur im aktuellen Package sichtbar, aber eigentlich könnte jedermann eine neue Operation implementieren.

In meinem Buch steht, dass Schnittstellen und deren Elemente automatisch als public gelten.

Ist das so (habs noch nie gehört, aber wenn es in dem Buch steht...)? Ich würde mich da einfach an die Mehrheit anpassen, und die macht das public :wink:

Die Variablen benötigst du gar nie. Klar, im Konstruktor benutzt du sie um die Buttons auf das Fenster zu kriegen, aber nur im Konstruktor. Keine andere Methode greifft auf diese Variablen zu. Also: weg damit.

Als lokale Variablen im Konstruktor einführn oder wie meinst das?

Genau so, mit lokalen Variablen.