1
\$\begingroup\$

I've wrote a calculator in Java with a GUI using swing. Here are the classes:

Control.java:

public class Control {
    public static void main(String[] args) {
        Calculator calculator = new Calculator();
        Gui gui = new Gui(calculator);
        gui.setVisible(true);
        boolean exit = false;
        while(!exit) {

        }
    }
}

Gui.java:

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.util.logging.Level;
import java.util.logging.Logger;

public class Gui extends JFrame {
    Calculator calculator;
    public Gui(Calculator calculator) {
        this.calculator = calculator;
        //The frame
        setSize(400, 400);
        setDefaultCloseOperation(EXIT_ON_CLOSE);

        //Panel for Text-output
        JPanel textPanel = new JPanel();
        textPanel.setPreferredSize(new Dimension(400,70));
        JTextField textfield = new JTextField("");
        textfield.setPreferredSize(new Dimension(380,70));
        textfield.setHorizontalAlignment(JTextField.RIGHT);
        textPanel.add(textfield);
        add(textPanel, BorderLayout.NORTH);

        //Panel for buttons
        JPanel buttonPanel = new JPanel();
        buttonPanel.setLayout(new java.awt.GridLayout(4, 5));
        buttonPanel.setPreferredSize(new Dimension(380, 280));

        //Buttons
        String[] buttonText = {
            "7", "8", "9", "+", "<-", "4", "5", "6", "-", "(", "1", "2", "3",
            "*", ")", "0", ",", "C", "/", "="
        };
        JButton button[] = new JButton[20];
        for(int i = 0; i < 20; i++) {
            button[i] = new JButton(buttonText[i]);
            button[i].addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent event) {
                    for(int i = 0; i < 20; i++) {

                        if(event.getSource() == button[i]) {

                            //Delete last character button
                            if(i == 4) {
                                String text = textfield.getText().toString();
                                text = text.substring(0, text.length()-1);
                                textfield.setText(text);
                            }

                            //Delete all button
                            else if(i == 17) {
                                textfield.setText("");
                            }

                            //"="-button
                            else if(i == 19) {
                                String text = textfield.getText().toString();
                                String output = "";
                                try {
                                    output = calculator.calculate(text);
                                } 
                                catch (Exception ex) {

                                }
                                textfield.setText(output);  
                            }

                            //other buttons
                            else {
                                String text = button[i].getText().toString();
                                String fieldText = textfield.getText().toString();
                                textfield.setText(fieldText + text);
                            }

                        }
                    }
                }
            });
            buttonPanel.add(button[i]);
        }

        add(buttonPanel, BorderLayout.SOUTH);
    }   
}

Calculator.java:

import javax.script.ScriptEngine;
import javax.script.ScriptEngineManager;

public class Calculator {
    public String calculate(String text) throws Exception {
        ScriptEngineManager scriptEngineManager = new ScriptEngineManager();
        ScriptEngine scriptEngine = scriptEngineManager.getEngineByName("JavaScript");
        Object sol = scriptEngine.eval(text);
        String solution = sol + "";
        return solution;
    }
}

What do you think about it? Do you have any suggestions on improving the code?

\$\endgroup\$
1

2 Answers 2

1
\$\begingroup\$

First of all, you should read a bit on how to start a Swing application. You must take care of the Event Dispatcher Thread when starting your application. And, because of the way AWT manage threads, you don't need this infinite loop that will just take resources for nothing. The main thread will stay active until your frame is closed[1].

View

For the GUI, it is always a good idea to extract your components. You can create other classes, inner classes or factory methods to distinct your TextPanel, ButtonsPanel and the frame itself. Keep in mind the single responsibility principle.

Gui(Calculator calculator) {
    this.calculator = calculator;
    setSize(400, 400);
    setDefaultCloseOperation(EXIT_ON_CLOSE);

    add(text = new TextPanel(), BorderLayout.NORTH);
    add(buttons = new ButtonsPanel(text), BorderLayout.SOUTH);
}

The ButtonsPanel is the most complex part of your view. The buttons loop and the event handling should be improved. Let's start with the deepest for. Did you know that you can set one actionCommand on a JButton this will be useful to assign one value to one button and drop the loop inside the event handling.

But there is better, you can create your own action or button to clearly expose this aspect. To improve your code readability you can also introduce some constants for the special buttons. By doing that you remove the comparison on array indexes and have a code that start to be auto documented

ButtonsPanel(final TextPanel screen, final Calculator calculator) {
    super(new java.awt.GridLayout(4, 5));
    setPreferredSize(new Dimension(380, 280));

    for (int i = 0; i < buttonText.length; i++) {
        add(new ActionButton(buttonText[i]));
    }
}

private void onButtonPressed(String symbol) {
    if (ERASE.equals(symbol)) {
        String text = screen.getText();
        text = text.substring(0, text.length() - 1);
        screen.setText(text);
    }  else if (CLEAR.equals(symbol)) {
        screen.setText("");
    } else if (EQUAL.equals(symbol)) {
        String text = screen.getText();
        String output = "";
        try {
            output = calculator.calculate(text);
        } catch (Exception ex) {
        }
        screen.setText(output);
    } else {
        String fieldText = screen.getText();
        screen.setText(fieldText + symbol);
    }
}

At this time you can notice that most of your logic is into this ButtonsPanel. This is not a good thing because testing it is not easy and this class mixes presentation and logic.

The de-facto pattern for Swing is MVC, you have already noticed that all existing components are using listeners. And you can do the same. Create one listener that will be used by your Gui to be notified when one button is pressed. In the meantime, if you are using Java 9 or bigger, you can use a switch expression to replace your ifs.

public Gui(final Calculator calculator) {
    this.calculator = calculator;
    setSize(400, 400);
    setDefaultCloseOperation(EXIT_ON_CLOSE);
    add(display = new TextPanel(), BorderLayout.NORTH);
    add(buttons = new ButtonsPanel(display, calculator), BorderLayout.SOUTH);
    buttons.addListener(symbol -> {
        StringBuilder equation = new StringBuilder(display.getText());
        switch (symbol) {
            case ButtonsPanel.ERASE:
                equation.deleteCharAt(equation.length()-1);
                break;
            case ButtonsPanel.CLEAR:
                equation.delete(0, equation.length());
                break;
            case ButtonsPanel.EQUAL:
                String output = "";
                try {
                    output = calculator.calculate(equation.toString());
                } catch (Exception ex) {
                    // FIXME
                }
                equation.replace(0, equation.length(), output);
                break;
            default:
                equation.append(symbol);
        }
        display.setText(equation.toString());
    });
}

While this is still better, in my opinion, you are still exposing too much details onf your ButtonsPanel (what if you want to use icons instead of String). The ButtonsListener should have dedicated method for the special symbols.

interface ButtonsPanelListener extends EventListener {
    void onInput(String text);

    void onClearScreen();

    void onEraseOne();

    void onEqual();
}

By doing that you will move the interpretation of the symbols back to the ButtonsPanel. Another improvement that you can do is create a class for all your buttons with a callback to trigger the correct method on the listener. By doing that the behavior is attached to the button and the reader will not have to scroll into the sources to find how each button behave.

interface Key {
    String getText();
    void onPressed(ButtonsPanelListener listener);
}

This will clean the "view" part. But, as already said, Swing is an MVC framework. So you are still missing the "m" and "c" parts...

Model

The model must be where your "state" is stored. Ideally this is the only class where you set and get the text. Let's create an Equation class for it and add all the required method on it (append, clear, dropLast, ..)

So the Gui change this model when he receive an event from his ButtonsPanel. And you ends-up a line that is duplicate for each kind of event:

display.setText(equation.getText());

In Swing, the model is usually observable, so you can also add some listeners to react when he change. With this observable model your Gui will receive events form the buttons and update the model. When updated, the model will notify the text (via the event listener) and the text will change.

  +---------------+  +-----+     +-----------+    +-----------+   
  | ButtonsPanel  |  | Gui |     | Equation  |    | TextPanel |   
  +---------------+  +-----+     +-----------+    +-----------+   
          |             |              |                |         
          | onInput     |              |                |         
          |------------>|              |                |         
          |             |              |                |         
          |             | append       |                |         
          |             |------------->|                |         
          |             |              |                |         
          |             |              | onChange       |         
          |             |              |--------------->|         
          |             |              |                |         
          |             |              |                | setText 
          |             |              |                |-------- 
          |             |              |                |       | 
          |             |              |                |<------- 
          |             |              |                |     

You can notice that from one side, there is ane actor between the event and the model (the Gui between ButtonsPanel and Equation), while on the other side there is no actor between the model and one view (Equation to TextPanel). This incoherence lead us to a little architectural discussion.

It is up to you to decide who will listen and react to an event. But, in my case, I rely on the smart and dumb containers pattern. Where I have one smart component, the Gui and the two dumbs, ButtonsPanel and TextPanel. So that the Gui is a kind of mediator between the model and the views.

 +-----------+    +-----+       +---------------+ +-----------+
 | Equation  |    | Gui |       | ButtonsPanel  | | TextPanel |
 +-----------+    +-----+       +---------------+ +-----------+
       |             |                  |               |
       |             |         onInput  |               |
       |             |<-----------------|               |
       |             |                  |               |
       |      append |                  |               |
       |<------------|                  |               |
       |             |                  |               |
       | onChange    |                  |               |
       |------------>|                  |               |
       |             |                  |               |
       |             | setText          |               |
       |             |--------------------------------->|
       |             |                  |               |

In fact the Gui has too much responsibility, because it play both the role of a view (a smart container) and the role of a controller.

Controller

Let's segregate the roles of the Gui is role will be to deal with the presentation and redirect events to a controller. On the other side the controller will update the model according to the received actions.

In some systems the controller implements all the required listeners. This is sometime not the best one for code reuse because the controller depends on Swing. Also it require to expose the listeners of all of your components or create adapters between the listeners for the dumb components and those of the smart components. But it is useful to reduce the number of delegation or decoration methods. The biggest advantage is that it force you to create one model that represent the state of your application (or part of it) so that any change can be observed by the view.

Finally

A this time you should have a clean MVC. The advantages of this pattern is that you can easily test the model and the controller who are usually the most critical parts in an MVC. You can also extract your core business (representation and execution of an equation) to reusable and testable classes and build your model (and sometimes controller) over them by applying the decorator and adapter patterns.

Please note that there are other popular pattern to build one application, Model View Presenter is one of them.

 +-------+       +-------+              +---------------+ +-----------+ +-------------+
 | Model |       | View  |              | ButtonsPanel  | | TextPanel | | Controller  |
 +-------+       +-------+              +---------------+ +-----------+ +-------------+
     |               |                          |               |              |
     |               |         onButtonPressed  |               |              |
     |               |<-------------------------|               |              |
     |               |                          |               |              |
     |               | // call method for the pressed button    |              |
     |               |-------------------------------------------------------->|
     |               |                          |               |              |
     |               |                          |      // call mutation method |
     |<------------------------------------------------------------------------|
     |               |                          |               |              |
     | onChange      |                          |               |              |
     |-------------->|                          |               |              |
     |               |                          |               |              |
     |               | setText                  |               |              |
     |               |----------------------------------------->|              |
     |               |                          |               |              |
Model
class Model {

    private final EventListenerList listeners = new EventListenerList();
    private final StringBuilder content;

    public Model() {
        this.content = new StringBuilder();
    }

    public void setResult(String result) {
        fireOnResult(result);
    }

    public void append(String part) {
        content.append(part);
        fireOnChange();
    }

    public void clear() {
        content.delete(0, content.length());
        fireOnChange();
    }

    public void dropLast() {
        content.deleteCharAt(content.length());
        fireOnChange();
    }

    public String getText() {
        return content.toString();
    }

    public void addListener(ModelListener listener) {
        listeners.add(ModelListener.class, listener);
    }

    public void removeListener(ModelListener listener) {
        listeners.remove(ModelListener.class, listener);
    }

    private void fireOnChange() {
        ModelListener[] lstnrs = listeners.getListeners(ModelListener.class);
        for (int i=lstnrs.length-1; i > -1; i--) {
            lstnrs[i].onEquationChange(content.toString());
        }
    }

    private void fireOnResult(String result) {
        ModelListener[] lstnrs = listeners.getListeners(ModelListener.class);
        for (int i=lstnrs.length-1; i > -1; i--) {
            lstnrs[i].onResult(result);
        }
    }

}
View
class View extends JFrame {
    private final Controller controller;
    private final ButtonsPanel buttons;
    private final TextPanel display;

    public View(final Controller controller) {
        this.controller = controller;
        setSize(400, 400);
        setDefaultCloseOperation(EXIT_ON_CLOSE);

        add(display = new TextPanel(), BorderLayout.NORTH);
        add(buttons = new ButtonsPanel(), BorderLayout.SOUTH);

        buttons.addListener(new ButtonsListener());
        controller.addModelListener(new TextUpdater());
    }

    private final class TextUpdater implements ModelListener {
        private void updateText(String newText) {
            SwingUtilities.invokeLater(() -> display.setText(newText));
        }

        @Override
        public void onEquationChange(String equation) {
            updateText(equation);
        }

        @Override
        public void onResult(String result) {
            updateText(result);
        }
    }

    private final class ButtonsListener implements ButtonsPanel.ButtonsPanelListener {
        @Override
        public void onButtonPressed(String text) {
            new SwingWorker<Void, Void>(){
                @Override
                protected Void doInBackground() throws Exception {
                    switch (text) {
                        case ButtonsPanel.CLEAR:
                            controller.clear();
                            break;
                        case ButtonsPanel.ERASE:
                            controller.eraseOne();
                            break;
                        case ButtonsPanel.EQUAL:
                            controller.compute();
                            break;
                        default:
                            controller.onInput(text);
                    }
                    return null;
                }
            }.execute();
        }
    }

}
Controller
class Controller {

    private final Engine engine;
    private final Model model;

    public Controller(Engine engine) {
        this.engine = engine;
        this.model = new Model();
    }

    public void compute() throws Exception {
        String result = engine.compute(model.getText());
        model.setResult(result);
    }

    public void onInput(String text) {
        model.append(text);
    }


    public void clear() {
        model.clear();
    }


    public void eraseOne() {
        model.dropLast();
    }


    public void addModelListener(ModelListener listener) {
        model.addListener(listener);
    }

    public void removeModelListener(ModelListener listener) {
        model.removeListener(listener);
    }

}     

[1] : https://docs.oracle.com/javase/7/docs/api/java/awt/doc-files/AWTThreadIssues.html

The sequence diagrams were made with TextArt.io : https://textart.io/sequence

\$\endgroup\$
2
\$\begingroup\$

Control.java

I don't like this name as it's not specific. I'd suggest 'CalculatorApp' instead.

You don't need the inifinite loop. The application will already continue until the user closes the program.

// you can remove all of this:
boolean exit = false;
while(!exit) {
}

Calculator.java

If you didn't write the program, would you know what Calculator.calculate(String) does? The naming is very ambiguous and the code itself looks very complicated (thankfully the heavy complicated, heavy lifting is done by an external library). You should add a javadoc to both the class & method at the very least.

You could have done this entire assignment without JavaScript. You may have gotten more learning out of it if you didn't use ScriptEngine. To me it seems hacky. I do give you bonus points for cleverness, though.

Gui.java

You have 2 unused imports (Level & Logger)

Avoid using wildcards (.*) in your imports. It clutters the local namespace and It makes it harder to tell which libraries are doing what. It will also cause a compiler error when there are two classes with the same name.

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

If you're using an IDE you should see a warning about your class not having a serialversionUID. You can allow your IDE to auto-generate one for you:

private static final long serialVersionUID = 1L;

Avoid magic numbers & magic strings Your class would be easier to maintain & read if you used static variables declared at the top. E.G:

private static final int WIDTH = 400;
private static final int HEIGHT = 400;
...
//The frame
setSize(WIDTH, HEIGHT);

No need to count the number of values in an array yourself, instead use array.length. This will make maintenance easier as you don't have to re-count the values every time:

JButton button[] = new JButton[buttonText.length];
for(int i = 0; i < buttonText.length; i++) {

Alternatively use a 'for-each' loop since you don't need the index.

'17' should definitely be a variable here, as it's currently unreadable. Same goes for your other magic strings/variables:

else if(i == 17) {

This is probably one of, if not the worst thing you can possibly do. You may not think it matters now but trust me, this is a really bad habit and the best way to use good practices is to never learn the bad ones. Please use a throws declaration if you don't want to handle errors:

catch (Exception ex) {
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ In addition to this good review, Swing is not thread safe. The method body inside Control should all be done on the Event Dispatch Thread. (I.e., inside a call to SwingUtils.invokeLater()) \$\endgroup\$
    – markspace
    Jan 7, 2020 at 20:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.