Calculateur de swing Java MDAS de base
J'ai récemment commencé à apprendre Java et j'ai décidé de créer une calculatrice MDAS de base dans Swing. Je ne suis pas complètement novice en programmation mais je fais peut-être des erreurs courantes ou n'écris pas le code le plus efficace.
Je voulais faire une calculatrice qui peut prendre plusieurs nombres et opérations avant de trouver la réponse en utilisant MDAS, au lieu de simplement renvoyer la réponse après chaque opération et de l'utiliser pour la suivante.
par exemple 2 * 3 + 4 - 5 / 5 = 9 au lieu de 1
Mon code se compose d'une seule classe. Il n'y a pas beaucoup de code donc je ne savais pas s'il y avait une bonne raison de le diviser en plusieurs classes, mais je n'ai jamais écrit quelque chose comme ça, alors n'hésitez pas à me corriger.
Repo avec exemple gif et jar exécutable
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();
}
}
J'apprécierais tous les conseils.
Réponses
package calculator;
Les noms de package doivent associer le logiciel à l'auteur, par exemple com.github.razemoon.basicmdasjavacaluclator.
public class GUI extends JFrame {
Pour les conventions de nming Java, vous utiliseriez normalement UpperCamelCase, et utilisez des minuscules même pour les acronymes, comme "Gui", "HtmlWidgetToolkit" ou "HtmlCssParser".
private static final long serialVersionUID = 1L;
Vous n'avez besoin de ce champ que s'il est très probable que la classe sera sérialisée ... dans ce cas, très probablement pas.
final String[] ops = new String[] {"+", "-", "x", "/"};
Pourquoi cela package-private?
De plus, les finaltableaux ne sont pas comme finalvous le pensez, les valeurs individuelles peuvent toujours être modifiées. Vous voulez probablement un Enum ... en fait, vous voulez une interface, mais dans cet exemple, un Enum ferait probablement assez bien.
private ArrayList<Integer> numHistory = new ArrayList<Integer>();
private ArrayList<String> opHistory = new ArrayList<String>();
Essayez toujours d'utiliser l'interface commune la plus basse pour les déclarations, dans ce cas 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);
}
Pourquoi utilisez-vous thisici mais nulle part ailleurs?
this.setResizable(false);
Pourquoi? Votre cadre est parfaitement redimensionnable pour autant que je puisse voir. En le définissant comme non redimensionnable, vous vous assurez uniquement que votre application devient inutilisable sous différents LaF et tailles de police.
for (int i = 0; i < 10; i++) {
Je suis un défenseur très persistant du fait que vous ne pouvez utiliser des noms de variables à une seule lettre que si vous traitez avec des dimensions.
for (int number = 0; number <= 9; number++) {
// Or
for (int digit = 0; digit <= 9; digit++) {
final int num = i;
Ne raccourcissez pas les noms de variables simplement parce que vous le pouvez, la diminution de la saisie ne vaut pas la diminution de la lisibilité.
En ce qui concerne la création de boutons, j'aime créer des méthodes d'assistance et des classes qui facilitent la lecture du code, dans ce cas, j'opterais pour des lambdas, comme ceci:
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.
}));
Une autre alternative serait de créer un private class NumberActionqui accepte un nombre dans son constructeur et exécute l'action associée. Cela vous permettrait également de vous débarrasser de la redéclaration finale.
private int i;
C'est un très mauvais nom de variable.
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();
}
Il vaudrait mieux répartir les responsabilités ici. Le cadre lui-même n'est responsable que de sa propre mise en page, tandis que la méthode principale devrait être responsable de l'affichage du cadre.
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);
}
Votre logique ne semble contenir aucune sorte de gestion des erreurs, je crois que le fait d'appuyer deux fois de suite sur un bouton d'opérateur devrait générer une erreur.
Une meilleure approche serait peut-être d'imprimer l'expression entière à l'écran telle qu'elle a été saisie, puis d'appliquer l' algorithme Shunting Yard pour traiter cette expression.
Votre logique ne fait pas de décimales et ne gère pas non plus les débordements avec élégance. En changeant votre logique d'utilisation, BigDecimalvous pouvez gérer les deux facilement. Notez que vous devez créer des BigDecimals avec une MathContextprécision et un comportement appropriés.
Si vous voulez lire une implémentation déjà existante, je peux recommander exp4j pour une bibliothèque d'expressions mathématiques utilisant des flotteurs, EvalEx pour celle qui utilise BigDecimalet mon propre projet jMathPaper pour une calculatrice qui arbore différentes interfaces graphiques (en ce qui concerne l'abstraction).
Vous pouvez utiliser la déclaration de tableau.
Lorsque vous voulez un tableau avec des valeurs prédéfinies qui peuvent être constantes, vous pouvez déclarer le tableau de manière anonyme.
final String[] ops = {"+", "-", "x", "/"};
Utilisez Enums pour les opérations.
Au lieu d'avoir un tableau d'opérations, je vous suggère de créer un Enum à la place.
public enum Operators {
PLUS("+"), MINUS("-"), MUL("x"), DIV("/");
private final String operator;
Operators(String operator) {
this.operator = operator;
}
public String getOperator() {
return operator;
}
}
Cela vous donnera plus d'avantages que le tableau, car vous pourrez supprimer la duplication.
//[...]
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));
}
//[...]
De plus, à mon avis, cela rendra le code plus facile à travailler et à refactoriser.
Lors de la division, vérifiez toujours le divisoravant de procéder à la division.
En divisant par zéro, il y a un java.lang.ArithmeticExceptionlancer par java; Je vous suggère d'ajouter un chèque :)
Utilisez au Queuelieu de Listpour conserver l'historique.
En utilisant le, Listvous devez utiliser un index, le Queuepour supprimer le premier élément ( java.util.Queue#poll); le seul inconvénient, vous devrez refactoriser le code réel pour supprimer le fichier indexOf.
private Queue<String> opHistory = new ArrayDeque<>();
Ce faisant, vous réduirez le code.
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);
}
Extrayez l'expression en variables lorsqu'elle est utilisée plusieurs fois.
Dans votre code, vous pouvez extraire les expressions similaires en variables; cela rendra le code plus court et plus facile à lire.
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);
}