Calcolatrice swing Java MDAS di base
Recentemente ho iniziato a imparare Java e ho deciso di creare un calcolatore MDAS di base in Swing. Non sono completamente nuovo alla programmazione, ma potrei commettere alcuni errori comuni o non scrivere il codice più efficiente.
Volevo creare una calcolatrice in grado di prendere più numeri e operazioni prima di trovare la risposta utilizzando MDAS, invece di restituire semplicemente la risposta dopo ogni operazione e utilizzarla per la successiva.
ad esempio 2 * 3 + 4 - 5 / 5 = 9 invece di 1
Il mio codice è costituito da una singola classe. Non c'è molto codice quindi non sapevo se ci fosse un buon motivo per dividerlo in più classi, tuttavia non ho mai scritto qualcosa del genere, quindi sentiti libero di correggermi.
Repo con gif di esempio e jar eseguibile
package calculator;
import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.util.ArrayList;
import javax.swing.AbstractAction;
import javax.swing.BorderFactory;
import javax.swing.Box;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
public class GUI extends JFrame {
private static final long serialVersionUID = 1L;
private String title = "Basic MDAS Calculator";
private int currentNumber;
private JLabel displayLabel = new JLabel(String.valueOf(currentNumber), JLabel.RIGHT);
private JPanel panel = new JPanel();
private boolean isClear = true;
final String[] ops = new String[] {"+", "-", "x", "/"};
private ArrayList<Integer> numHistory = new ArrayList<Integer>();
private ArrayList<String> opHistory = new ArrayList<String>();
public GUI() {
setPanel();
setFrame();
}
private void setFrame() {
this.setTitle(title);
this.add(panel, BorderLayout.CENTER);
this.setBounds(10,10,300,700);
this.setResizable(false);
this.setVisible(true);
this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}
private void setPanel() {
panel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
panel.setLayout(new GridLayout(0, 1));
displayLabel.setFont(new Font("Verdana", Font.PLAIN, 42));
panel.add(displayLabel);
panel.add(Box.createRigidArea(new Dimension(0, 0)));
createButtons();
}
private void createButtons() {
// 0-9
for (int i = 0; i < 10; i++) {
final int num = i;
JButton button = new JButton( new AbstractAction(String.valueOf(i)) {
private static final long serialVersionUID = 1L;
@Override
public void actionPerformed(ActionEvent e) {
// If somebody presses "=" and then types a number, start a new equation instead
// of adding that number to the end like usual
if (!isClear) {
currentNumber = 0;
isClear = true;
}
if (currentNumber == 0) {
currentNumber = num;
} else {
currentNumber = currentNumber * 10 + num;
}
displayLabel.setText(String.valueOf(currentNumber));
}
});
panel.add(button);
}
// +, -, x, /
for (String op : ops) {
JButton button = new JButton( new AbstractAction(op) {
private static final long serialVersionUID = 1L;
@Override
public void actionPerformed(ActionEvent e) {
numHistory.add(currentNumber);
currentNumber = 0;
opHistory.add(op);
displayLabel.setText(op);
}
});
panel.add(button);
}
// =
JButton button = new JButton( new AbstractAction("=") {
private static final long serialVersionUID = 1L;
private int i;
@Override
public void actionPerformed(ActionEvent e) {
// Display result
numHistory.add(currentNumber);
while (opHistory.size() > 0) {
if (opHistory.contains("x")) {
i = opHistory.indexOf("x");
numHistory.set(i, numHistory.get(i) * numHistory.get(i+1));
} else if (opHistory.contains("/")) {
i = opHistory.indexOf("/");
numHistory.set(i, numHistory.get(i) / numHistory.get(i+1));
} else if (opHistory.contains("+")) {
i = opHistory.indexOf("+");
numHistory.set(i, numHistory.get(i) + numHistory.get(i+1));
} else if (opHistory.contains("-")) {
i = opHistory.indexOf("-");
numHistory.set(i, numHistory.get(i) - numHistory.get(i+1));
}
opHistory.remove(i);
numHistory.remove(i+1);
}
displayLabel.setText(String.valueOf(numHistory.get(0)));
currentNumber = numHistory.get(0);
numHistory.clear();
if (isClear) {
isClear = false;
}
}
});
panel.add(button);
}
public static void main(String[] args) {
new GUI();
}
}
Apprezzerei eventuali suggerimenti.
Risposte
package calculator;
I nomi dei pacchetti dovrebbero associare il software all'autore, come com.github.razemoon.basicmdasjavacaluclator.
public class GUI extends JFrame {
Per le convenzioni di nming Java, normalmente useresti UpperCamelCase e userai le lettere minuscole anche per gli acronimi, come "Gui", "HtmlWidgetToolkit" o "HtmlCssParser".
private static final long serialVersionUID = 1L;
Questo campo è necessario solo se è molto probabile che la classe venga serializzata ... in questo caso, molto probabilmente no.
final String[] ops = new String[] {"+", "-", "x", "/"};
Perché è questo package-private?
Inoltre, gli finalarray non sono come finalpenseresti, i singoli valori possono ancora essere modificati. Molto probabilmente vuoi un Enum ... in realtà, vuoi un'interfaccia, ma in questo esempio, un Enum molto probabilmente andrebbe abbastanza bene.
private ArrayList<Integer> numHistory = new ArrayList<Integer>();
private ArrayList<String> opHistory = new ArrayList<String>();
In questo caso, cerca sempre di utilizzare l'interfaccia comune più bassa per le dichiarazioni List.
private void setFrame() {
this.setTitle(title);
this.add(panel, BorderLayout.CENTER);
this.setBounds(10,10,300,700);
this.setResizable(false);
this.setVisible(true);
this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}
Perché stai usando thisqui ma da nessun'altra parte?
this.setResizable(false);
Perché? La tua cornice è perfettamente ridimensionabile per quanto posso vedere. Impostandolo non ridimensionabile ti assicuri solo che la tua applicazione diventi inutilizzabile con diversi LaF e dimensioni dei caratteri.
for (int i = 0; i < 10; i++) {
Sono un sostenitore molto ostinato del fatto che ti è permesso usare nomi di variabili di una sola lettera se hai a che fare con le dimensioni.
for (int number = 0; number <= 9; number++) {
// Or
for (int digit = 0; digit <= 9; digit++) {
final int num = i;
Non abbreviare i nomi delle variabili solo perché puoi, la minore quantità di digitazione non vale la minore leggibilità.
Per quanto riguarda la creazione di pulsanti, mi piace creare metodi e classi di supporto che rendano il codice più facile da leggere, in questo caso sceglierei lambda, come questo:
private JButton createButton(String text, Runnable action) {
return new JButton(new AbstractButton(text) {
@Override
public void actionPerformed(ActionEvent e) {
action.run();
}
})
}
// In createButtons:
panel.add(createButton(Integer.toString(number), () -> {
// Code for the number button goes here.
}));
Un'altra alternativa sarebbe creare un private class NumberActionche accetti un numero nel suo costruttore ed esegua l'azione associata. Ciò consentirebbe anche di sbarazzarsi della ridichiarazione finale.
private int i;
Questo è un pessimo nome di variabile.
public GUI() {
setPanel();
setFrame();
}
private void setFrame() {
this.setTitle(title);
this.add(panel, BorderLayout.CENTER);
this.setBounds(10,10,300,700);
this.setResizable(false);
this.setVisible(true);
this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}
// ...
public static void main(String[] args) {
new GUI();
}
Sarebbe meglio dividere le responsabilità qui. Il frame stesso è responsabile solo della creazione del proprio layout, mentre il metodo principale dovrebbe essere responsabile della visualizzazione del frame.
public GUI() {
setPanel();
setFrame();
}
private void setFrame() {
this.setTitle(title);
this.add(panel, BorderLayout.CENTER);
this.setBounds(10,10,300,700);
this.setResizable(false);
}
// ...
public static void main(String[] args) {
GUI gui = new GUI();
gui.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
gui.setVisible(true);
}
La tua logica non sembra contenere alcun tipo di gestione degli errori, credo che premendo un pulsante dell'operatore due volte di seguito dovrebbe restituire un errore.
Forse un approccio migliore sarebbe stampare l'intera espressione sullo schermo così come è stata inserita, quindi applicare l' algoritmo Shunting Yard per elaborare quell'espressione.
La tua logica non fa i decimali, né gestisce con garbo gli overflow. Modificando la logica che usi BigDecimal, potresti gestirli entrambi facilmente. Notare che è necessario creare messaggi di posta BigDecimalelettronica appropriati MathContextper avere una corretta precisione e comportamento.
Se vuoi leggere un'implementazione già esistente, posso consigliare exp4j per una libreria di espressioni matematiche che utilizza float, EvalEx per uno che utilizza BigDecimale il mio progetto jMathPaper per una calcolatrice che utilizza diverse GUI (per quanto riguarda l'astrazione).
È possibile utilizzare la dichiarazione dell'array.
Quando si desidera un array con valori predefiniti che possono essere costanti, è possibile dichiarare l'array in modo anonimo.
final String[] ops = {"+", "-", "x", "/"};
Usa Enum per le operazioni.
Invece di avere un array di operazioni, ti suggerisco di creare invece un Enum.
public enum Operators {
PLUS("+"), MINUS("-"), MUL("x"), DIV("/");
private final String operator;
Operators(String operator) {
this.operator = operator;
}
public String getOperator() {
return operator;
}
}
Questo ti darà un vantaggio maggiore rispetto all'array, poiché sarai in grado di rimuovere la duplicazione.
//[...]
for (Operators op : Operators.values()) {
JButton button = new JButton( new AbstractAction(op.getOperator()) {
private static final long serialVersionUID = 1L;
@Override
public void actionPerformed(ActionEvent e) {
numHistory.add(currentNumber);
currentNumber = 0;
opHistory.add(op);
displayLabel.setText(String.valueOf(op.getOperator()));
}
});
panel.add(button);
}
//[...]
//[...]
if (opHistory.contains(Operators.MUL)) {
i = opHistory.indexOf(Operators.MUL);
numHistory.set(i, numHistory.get(i) * numHistory.get(i + 1));
} else if (opHistory.contains(Operators.DIV)) {
i = opHistory.indexOf(Operators.DIV);
numHistory.set(i, numHistory.get(i) / numHistory.get(i + 1));
} else if (opHistory.contains(Operators.PLUS)) {
i = opHistory.indexOf(Operators.PLUS);
numHistory.set(i, numHistory.get(i) + numHistory.get(i + 1));
} else if (opHistory.contains(Operators.MINUS)) {
i = opHistory.indexOf(Operators.MINUS);
numHistory.set(i, numHistory.get(i) - numHistory.get(i + 1));
}
//[...]
Inoltre, a mio parere, questo renderà il codice più facile da lavorare e refactoring.
Quando si divide, controllare sempre divisorprima di eseguire la divisione.
Quando si divide per zero, c'è un java.lang.ArithmeticExceptionlancio di java; Ti suggerisco di aggiungere un segno di spunta :)
Usa il Queueinvece di Listper mantenere la cronologia.
Usando il Listdevi usare un indice, il Queueper rimuovere il primo elemento ( java.util.Queue#poll); l'unico inconveniente, sarà necessario refactoring del codice effettivo per rimuovere il file indexOf.
private Queue<String> opHistory = new ArrayDeque<>();
In questo modo, renderai il codice più breve.
while (opHistory.size() > 0) {
Operators currentOperator = opHistory.poll();
switch (currentOperator) { //Java 14+ Switch, you can use if or the older version of the switch.
case MUL -> numHistory.set(i, numHistory.get(i) * numHistory.get(i+1));
case DIV -> numHistory.set(i, numHistory.get(i) / numHistory.get(i+1));
case PLUS -> numHistory.set(i, numHistory.get(i) + numHistory.get(i+1));
case MINUS -> numHistory.set(i, numHistory.get(i) - numHistory.get(i+1));
}
numHistory.remove(i + 1);
}
Estrae l'espressione nelle variabili quando viene utilizzata più volte.
Nel tuo codice puoi estrarre le espressioni simili in variabili; questo renderà il codice più breve e più facile da leggere.
final Integer first = numHistory.get(i);
final Integer second = numHistory.get(i + 1);
switch (currentOperator) {
case MUL -> numHistory.set(i, first * second);
case DIV -> numHistory.set(i, first / second);
case PLUS -> numHistory.set(i, first + second);
case MINUS -> numHistory.set(i, first - second);
}