Podstawowy kalkulator wahań Java MDAS
Niedawno zacząłem uczyć się języka Java i postanowiłem zrobić podstawowy kalkulator MDAS w Swing. Nie jestem zupełnie nowy w programowaniu, ale być może popełniam kilka typowych błędów lub nie piszę najbardziej wydajnego kodu.
Chciałem stworzyć kalkulator, który może przyjmować wiele liczb i operacji przed znalezieniem odpowiedzi za pomocą MDAS, zamiast po prostu zwracać odpowiedź po każdej operacji i używać jej do następnej.
np. 2 * 3 + 4 - 5 / 5 = 9 zamiast 1
Mój kod składa się z jednej klasy. Nie ma zbyt wiele kodu, więc nie wiedziałem, czy istnieje dobry powód, aby podzielić go na wiele klas, jednak nigdy nie napisałem czegoś takiego, więc nie krępuj się mnie poprawić.
Repozytorium z przykładowym gifem i 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();
}
}
Byłbym wdzięczny za wszelkie wskazówki.
Odpowiedzi
package calculator;
Nazwy pakietów powinny kojarzyć oprogramowanie z autorem, np com.github.razemoon.basicmdasjavacaluclator..
public class GUI extends JFrame {
W przypadku konwencji Java Nming normalnie używasz UpperCamelCase i małych liter nawet w akronimach, takich jak „Gui”, „HtmlWidgetToolkit” lub „HtmlCssParser”.
private static final long serialVersionUID = 1L;
Potrzebujesz tego pola tylko wtedy, gdy jest wysoce prawdopodobne, że klasa zostanie serializowana ... w tym przypadku najprawdopodobniej nie.
final String[] ops = new String[] {"+", "-", "x", "/"};
Dlaczego tak się dzieje package-private?
Ponadto finaltablice nie są takie, finaljak myślisz, poszczególne wartości można nadal zmieniać. Najprawdopodobniej chcesz Enum ... w rzeczywistości potrzebujesz interfejsu, ale w tym przykładzie Enum najprawdopodobniej będzie wystarczająco dobry.
private ArrayList<Integer> numHistory = new ArrayList<Integer>();
private ArrayList<String> opHistory = new ArrayList<String>();
W tym przypadku zawsze staraj się używać najniższego wspólnego interfejsu dla deklaracji 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);
}
Dlaczego używasz thistutaj, ale nigdzie indziej?
this.setResizable(false);
Czemu? O ile widzę, twoja ramka jest idealnie skalowalna. Ustawiając niemożliwą do zmiany rozmiaru, upewniasz się tylko, że twoja aplikacja stanie się bezużyteczna w przypadku różnych LaF i rozmiarów czcionek.
for (int i = 0; i < 10; i++) {
Jestem bardzo wytrwałym zwolennikiem tego, że możesz używać tylko jednoliterowych nazw zmiennych, jeśli masz do czynienia z wymiarami.
for (int number = 0; number <= 9; number++) {
// Or
for (int digit = 0; digit <= 9; digit++) {
final int num = i;
Nie skracaj nazw zmiennych tylko dlatego, że możesz, mniejsza ilość wpisywania nie jest warta zmniejszonej czytelności.
Jeśli chodzi o tworzenie przycisków, lubię tworzyć metody pomocnicze i klasy, które ułatwiają czytanie kodu, w tym przypadku wybrałbym lambdy, takie jak to:
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.
}));
Inną alternatywą byłoby utworzenie, private class NumberActionktóry akceptuje liczbę w swoim konstruktorze i wykonuje powiązaną akcję. Pozwoliłoby ci to również pozbyć się ponownej deklaracji końcowej.
private int i;
To bardzo zła nazwa zmiennej.
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();
}
Tutaj lepiej byłoby podzielić obowiązki. Sama ramka jest odpowiedzialna tylko za uruchomienie własnego układu, podczas gdy główna metoda powinna odpowiadać za wyświetlenie ramki.
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);
}
Twoja logika nie wydaje się zawierać żadnego rodzaju obsługi błędów, uważam, że dwukrotne naciśnięcie przycisku operatora powinno spowodować błąd.
Może lepszym podejściem byłoby wydrukowanie całego wyrażenia na ekranie, tak jak zostało wprowadzone, a następnie zastosowanie algorytmu manewrowania do przetworzenia tego wyrażenia.
Twoja logika nie robi ułamków dziesiętnych, ani też nie radzi sobie wdzięcznie z przepełnieniami. Zmieniając logikę, której używasz, BigDecimalmożesz łatwo sobie z nimi poradzić. Zauważ, że musisz tworzyć BigDecimals z odpowiednią MathContextdokładnością i zachowaniem.
Jeśli chcesz przeczytać już istniejącą implementację, mogę polecić exp4j dla biblioteki wyrażeń matematycznych używających floatów , EvalEx do jednego użycia BigDecimali mój własny projekt jMathPaper dla kalkulatora, który ma różne GUI (dotyczące abstrakcji).
Możesz użyć deklaracji tablicy.
Jeśli chcesz mieć tablicę ze wstępnie zdefiniowanymi wartościami, które mogą być stałe, możesz zadeklarować tablicę anonimowo.
final String[] ops = {"+", "-", "x", "/"};
Użyj wyliczeń do operacji.
Zamiast mieć tablicę operacji, sugeruję, aby zamiast tego utworzyć Enum.
public enum Operators {
PLUS("+"), MINUS("-"), MUL("x"), DIV("/");
private final String operator;
Operators(String operator) {
this.operator = operator;
}
public String getOperator() {
return operator;
}
}
To da ci większą przewagę niż macierz, ponieważ będziesz w stanie usunąć duplikację.
//[...]
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));
}
//[...]
Ponadto, moim zdaniem, ułatwi to pracę z kodem i jego refaktoryzację.
Podczas dzielenia zawsze sprawdzaj divisorprzed wykonaniem podziału.
Podczas dzielenia przez zero pojawia się java.lang.ArithmeticExceptionrzut przez java; Proponuję dodać czek :)
Użyj Queuezamiast, Listaby zachować historię.
Używając opcji List, musisz użyć indeksu, Queueaby usunąć pierwszą pozycję ( java.util.Queue#poll); jedyną wadą jest to, że będziesz musiał refaktoryzować rzeczywisty kod, aby usunąć indexOf.
private Queue<String> opHistory = new ArrayDeque<>();
W ten sposób skrócisz kod.
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);
}
Wyodrębnij wyrażenie do zmiennych, gdy jest używane wielokrotnie.
W swoim kodzie możesz wyodrębnić podobne wyrażenia do zmiennych; dzięki temu kod będzie krótszy i łatwiejszy do odczytania.
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);
}