Базовый калькулятор MDAS Java Swing
Я недавно начал изучать Java и решил сделать базовый калькулятор MDAS на Swing. Я не новичок в программировании, но, возможно, делаю некоторые типичные ошибки или пишу не самый эффективный код.
Я хотел сделать калькулятор, который может выполнять несколько чисел и операций перед поиском ответа с помощью MDAS, вместо того, чтобы просто возвращать ответ после каждой операции и использовать его для следующей.
например, 2 * 3 + 4 - 5 / 5 = 9 вместо 1
Мой код состоит из одного класса. Кода не так много, поэтому я не знал, есть ли веская причина для разделения его на несколько классов, однако я никогда не писал ничего подобного, поэтому, пожалуйста, не стесняйтесь поправлять меня.
Репо с примером gif и runnable jar
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();
}
}
Буду признателен за любые советы.
Ответы
package calculator;
Названия пакетов должны связывать программу с автором, например com.github.razemoon.basicmdasjavacaluclator.
public class GUI extends JFrame {
В соглашениях о языке Java обычно используется UpperCamelCase, а строчные буквы даже для аббревиатур, таких как «Gui», «HtmlWidgetToolkit» или «HtmlCssParser».
private static final long serialVersionUID = 1L;
Это поле нужно только в том случае, если высока вероятность того, что класс будет сериализован ... в этом случае, скорее всего, нет.
final String[] ops = new String[] {"+", "-", "x", "/"};
Почему это package-private?
Кроме того, finalмассивы не такие, finalкак вы думаете, отдельные значения все еще можно изменить. Скорее всего, вам нужен Enum ... на самом деле вам нужен интерфейс, но в этом примере Enum, скорее всего, подойдет.
private ArrayList<Integer> numHistory = new ArrayList<Integer>();
private ArrayList<String> opHistory = new ArrayList<String>();
В этом случае всегда старайтесь использовать самый нижний общий интерфейс для объявлений 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);
}
Почему вы используете thisздесь, а больше нигде?
this.setResizable(false);
Почему? Насколько я понимаю, размер вашей рамки можно изменять. Установив его без изменения размера, вы только убедитесь, что ваше приложение станет непригодным для использования при других LaF и других размерах шрифта.
for (int i = 0; i < 10; i++) {
Я очень настойчивый сторонник того, что вам разрешено использовать только однобуквенные имена переменных, если вы имеете дело с измерениями.
for (int number = 0; number <= 9; number++) {
// Or
for (int digit = 0; digit <= 9; digit++) {
final int num = i;
Не сокращайте имена переменных только потому, что это возможно, уменьшение объема ввода не стоит снижения удобочитаемости.
Что касается создания кнопок, мне нравится создавать вспомогательные методы и классы, которые упрощают чтение кода, в этом случае я бы выбрал лямбды, например:
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.
}));
Другой альтернативой было бы создание, private class NumberActionкоторое принимает число в своем конструкторе и выполняет связанное действие. Это также позволит вам избавиться от окончательного повторного объявления.
private int i;
Это очень плохое имя переменной.
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();
}
Здесь было бы лучше разделить обязанности. Сам фрейм отвечает только за создание собственного макета, в то время как основной метод должен отвечать за отображение фрейма.
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);
}
Ваша логика, похоже, не содержит никакой обработки ошибок, я считаю, что нажатие кнопки оператора дважды подряд должно привести к ошибке.
Возможно, лучшим подходом было бы напечатать все выражение на экране в том виде, в каком оно было введено, а затем применить алгоритм маневровой верфи для обработки этого выражения.
Ваша логика не использует десятичные дроби и не изящно обрабатывает переполнения. Изменив свою логику использования, BigDecimalвы легко справитесь с обоими. Обратите внимание, что вы должны создать BigDecimals с подходящей MathContextточностью и поведением.
Если вы хотите прочитать уже существующую реализацию, я могу порекомендовать exp4j для библиотеки математических выражений с использованием чисел с плавающей запятой, EvalEx для одного использования BigDecimalи мой собственный проект jMathPaper для калькулятора, который поддерживает различные графические интерфейсы (в отношении абстракции).
Вы можете использовать объявление массива.
Если вам нужен массив с предопределенными значениями, которые могут быть постоянными, вы можете объявить массив анонимно.
final String[] ops = {"+", "-", "x", "/"};
Используйте Enums для операций.
Вместо того, чтобы иметь массив операций, я предлагаю вам вместо этого создать Enum.
public enum Operators {
PLUS("+"), MINUS("-"), MUL("x"), DIV("/");
private final String operator;
Operators(String operator) {
this.operator = operator;
}
public String getOperator() {
return operator;
}
}
Это даст вам больше преимуществ, чем массив, поскольку вы сможете удалить дублирование.
//[...]
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));
}
//[...]
Также, на мой взгляд, это упростит работу с кодом и его рефакторинг.
При делении всегда проверяйте, divisorпрежде чем делать деление.
При делении на ноль возникает java.lang.ArithmeticExceptionбросок по java; Предлагаю добавить чек :)
Используйте Queueвместо, Listчтобы сохранить историю.
При использовании Listвы должны использовать индекс, Queueчтобы удалить первый элемент ( java.util.Queue#poll); единственный недостаток, вам нужно будет реорганизовать реальный код, чтобы удалить indexOf.
private Queue<String> opHistory = new ArrayDeque<>();
Тем самым вы сделаете код короче.
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);
}
Извлеките выражение в переменные при многократном использовании.
В своем коде вы можете извлекать похожие выражения в переменные; это сделает код короче и легче читается.
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);
}