7
\$\begingroup\$

This is for a high school class I'm taking. Any tips/improvements?

import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.script.ScriptEngine;
import javax.script.ScriptEngineManager;
import javax.script.ScriptException;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;

public class JavaCalculator
{
    public static boolean isMemorized;
    public static String memory;

    public static void main (String[] args)
    {
        JTextField answerField = new JTextField();
        answerField.setBounds (6, 6, 280, 48);
        answerField.setFont (new Font ("Arial", Font.PLAIN, 26));
        answerField.setColumns (1);
        answerField.setHorizontalAlignment (JTextField.RIGHT);
        answerField.setEditable (false);

        JButton button_one = new JButton ("1");
        button_one.setBounds (6, 92, 70, 38);
        button_one.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "1");
            }
        });

        JButton button_two = new JButton ("2");
        button_two.setBounds (76, 92, 70, 38);
        button_two.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "2");
            }
        });

        JButton button_three = new JButton ("3");
        button_three.setBounds (146, 92, 70, 38);
        button_three.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "3");
            }
        });

        JButton button_four = new JButton ("4");
        button_four.setBounds (6, 128, 70, 38);
        button_four.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "4");
            }
        });

        JButton button_five = new JButton ("5");
        button_five.setBounds (76, 128, 70, 38);
        button_five.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "5");
            }
        });

        JButton button_six = new JButton ("6");
        button_six.setBounds (146, 128, 70, 38);
        button_six.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "6");
            }
        });

        JButton button_seven = new JButton ("7"); 
        button_seven.setBounds (6, 164, 70, 38);
        button_seven.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "7");
            }
        });

        JButton button_eight = new JButton ("8");
        button_eight.setBounds (76, 164, 70, 38);
        button_eight.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "8");
            }
        });

        JButton button_nine = new JButton ("9");
        button_nine.setBounds (146, 164, 70, 38);
        button_nine.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "9");
            }
        });

        JButton button_zero = new JButton ("0");
        button_zero.setBounds (6, 200, 140, 50);
        button_zero.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + "0");
            }
        });

        JButton button_period = new JButton (".");
        button_period.setBounds (146, 200, 70, 50);
        button_period.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + ".");
            }
        });

        JButton button_plus = new JButton ("+");
        button_plus.setBounds (216, 128, 70, 38);
        button_plus.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + " + ");
            }
        });

        JButton button_minus = new JButton ("-");
        button_minus.setBounds (216, 92, 70, 38);
        button_minus.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + " - ");
            }
        });

        JButton button_times = new JButton ("*");
        button_times.setBounds (146, 56, 70, 38);
        button_times.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + " * ");
            }
        });

        JButton button_divide = new JButton ("/");
        button_divide.setBounds(216, 56, 70, 38);
        button_divide.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                answerField.setText (answerField.getText() + " / ");
            }
        });

        JButton button_equals = new JButton ("=");
        button_equals.setBounds (216, 164, 70, 88);
        button_equals.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent event)
            {
                ScriptEngineManager manager = new ScriptEngineManager();
                ScriptEngine engine = manager.getEngineByName ("JavaScript");

                try
                {
                    answerField.setText (answerField.getText() + " = " + engine.eval (answerField.getText()));
                }
                catch (ScriptException exception)
                {
                    exception.printStackTrace();
                }
            }
        });

        JButton button_memory = new JButton ("M");
        button_memory.setBounds (76, 56, 70, 38);
        button_memory.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent e)
            {
                if (!isMemorized)
                {
                    memory = answerField.getText();
                    button_memory.setText ("R");
                    isMemorized = true;
                }
                else if (isMemorized)
                {
                    answerField.setText (answerField.getText() + memory);
                }
            }
        });

        JButton button_clear = new JButton ("C");
        button_clear.setBounds (6, 56, 70, 38);
        button_clear.addActionListener (new ActionListener()
        {
            public void actionPerformed (ActionEvent e)
            {
                answerField.setText ("");
                button_memory.setText ("M");
                isMemorized = false;
            }
        });

        JPanel panel = new JPanel();
        panel.setLayout (null);
        panel.add (answerField);
        panel.add (button_one);
        panel.add (button_two);
        panel.add (button_three);
        panel.add (button_four);
        panel.add (button_five);
        panel.add (button_six);
        panel.add (button_seven);
        panel.add (button_eight);
        panel.add (button_nine);
        panel.add (button_zero);
        panel.add (button_period);
        panel.add (button_plus);
        panel.add (button_minus);
        panel.add (button_times);
        panel.add (button_divide);
        panel.add (button_equals);
        panel.add (button_memory);
        panel.add (button_clear);

        JFrame frame = new JFrame ("Java Calculator");
        frame.setDefaultCloseOperation (JFrame.EXIT_ON_CLOSE);
        frame.setSize (300, 278);
        frame.setResizable (false);
        frame.getContentPane().add (panel);
        frame.setVisible (true);
    }
}
\$\endgroup\$
6
\$\begingroup\$

Avoid copy-paste coding

An easy first step is to generalize this action listener:

button_two.addActionListener (new ActionListener()
{
    public void actionPerformed (ActionEvent event)
    {
        answerField.setText (answerField.getText() + "2");
    }
});

For example by creating a dedicated inner class for this:

private class NumberActionListener implements ActionListener {
    private static final label;

    public NumberActionListener(String label) {
        this.label = label;
    }

    @Override
    public void actionPerformed(ActionEvent event) {
        answerField.setText(answerField.getText() + label);
    }
}

Using this class, you can simplify the addActionListener calls like this:

button_two.addActionListener(new NumberActionListener("2"));

On closer look, I see that the action listeners of the operators do almost the same thing as the number buttons, but using some padding with space. You could improve on the NumberActionListener to handle that case too.

Find a formula to replace hardcoded numbers

The positioning of the buttons clearly follows some kind of logic, but the values are hardcoded, for example:

    button_one.setBounds (6, 92, 70, 38);
    button_two.setBounds (76, 92, 70, 38);
    button_three.setBounds (146, 92, 70, 38);
    button_four.setBounds (6, 128, 70, 38);
    button_five.setBounds (76, 128, 70, 38);
    button_six.setBounds (146, 128, 70, 38);

As it is, this is a maintenance nightmare. If you want to resize the buttons, or increase padding in between, you'll have to recalculate and change in many places. It would be better to come up with a formula, and position these buttons using a loop.

\$\endgroup\$
3
\$\begingroup\$

The main method should really just create an instance of your application, not contain all logic. In Java it is typical to extend JFrame and add all the buttons etc in the constructor. This gets rid of the nasty static variables.

Rather than setting the bounds of all the buttons directly it's better to use a LayoutManager of some sort. See the visual guide for some ideas.

A lot of your action listeners do the same thing, adding text to your textfield. You could factor out the common bits.

Calling out to javascript to do your maths is a bit weird. I'd expect someone to actually write code to do the maths.

\$\endgroup\$
  • 1
    \$\begingroup\$ typical to extend JFrame? Yes. Recommended to do so? Actually not. In most cases (can't think of any exceptions right now), it is better to use JFrame than extend it. Goes along with "prefer composition over inheritance". \$\endgroup\$ – Simon Forsberg Apr 30 '15 at 20:53
2
\$\begingroup\$

Layouts

I agree with @Simon's comment to use a layout manager, so that you don't need to use hard-coded numbers for positioning with the added benefit of freely resizing your application without worrying about alignment.

Dealing with repetition

Another way of getting the button text that triggered the ActionListener is the following:

public void actionPerformed(ActionEvent event) {
    JButton sourceButton = (JButton) event.getSource();
    answerField.setText(answerField.getText() + sourceButton.getText());
}

Going this way means you don't really need to re-specify your buttons' text and the values you want to store differently. If you happen to be on Java 8, the functional interfaces feature mean that your ActionListener need not be more than:

buttonX.addActionListener(event -> {
    appendText(((JButton) event.getSource()).getText());
});

Where appendText() is a simple wrapper method:

private void appendText(String value) {
    answerField.setText(answerField.getText() + value);
}

The idea behind this being to reduce the amount of repetitive code.

Calculation (and validation)

I think it's fine to get a ScriptEngine and then eval() the mathematical expression, but this two-step process is not as instantaneous. Therefore, I will prefer to store engine as a class-level field, and pressing the = button will only perform the evaluation. You may want to do a dummy evaluation first so that the very first calculation performed appears faster.

As you should have assessed by now, providing a reduced set of mathematical operators and doing calculations are easy, but validating and accepting a simple mathematical expression... not so much. Shameless plug alert: my take on a GUI calculator attempts to resolve that by enabling or disabling buttons depending on the existing input, so you may want to consider something like that. In that case, you wouldn't have to be concerned about a user entering + - * / =.

Should ScriptEngine throw an Exception related to your text field (e.g. when users absent-mindedly press the next expression to give you the following 'garbage' input: 1 + 2 = 34 + 5<= error, do not compute>), it will be more helpful to display it within your GUI, instead of just exception.printStackTrace(). This prepares you for dealing with Exceptions properly in the future, too.

Memorization

This is where you run into two usability-breaking problems:

  1. You memorize the entire text field instead of the previous result
  2. You don't have two separate buttons to separate the features of clearing the screen or the memory

Effectively, this means your memorization is broken as I can't reuse my past result! In order to enter a new mathematical expression to use the (R)ecall feature, I have to press (C)lear... which clears the memory too. Without fixing this (by implementing point 2), it's as good as not providing any memorization at all. :(

One small bit of code improvement suggestion, you don't need an else if (negation-of-if-boolean-condition) following an if statement. That if statement you have for your (M)emory button should be just:

if (isMemorized) {
    ...
} else {
    ...
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy