12
\$\begingroup\$

I used a standard calculator design from Java. I wanted to expand it so I created a class to create buttons for different operations, like +, -, *, /. The original program didn't do this, they just made them individually without a template method. Do you think I should do it like this?

//The java Template Calculator TODO

import java.awt.EventQueue;
import java.awt.GridLayout;
import java.awt.BorderLayout;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.JButton;
import java.awt.Container;

public class JavaCalculator implements ActionListener{

JFrame guiFrame;
JPanel buttonPanel;
JTextField numberCalc;
int calcOperation = 0;
int currentCalc;

public static void main(String[] args) {

     EventQueue.invokeLater(new Runnable()
     {

         public void run()
         {

             new JavaCalculator();         
         }
     });

}

public JavaCalculator()
{
    guiFrame = new JFrame();
    guiFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    guiFrame.setTitle("Simple Calculator");
    guiFrame.setSize(300,300);
    guiFrame.setLocationRelativeTo(null);
    numberCalc = new JTextField();
    numberCalc.setHorizontalAlignment(JTextField.RIGHT);
    numberCalc.setEditable(false);
    guiFrame.add(numberCalc, BorderLayout.NORTH);
    buttonPanel = new JPanel();
    buttonPanel.setLayout(new GridLayout(4,4));   
    guiFrame.add(buttonPanel, BorderLayout.CENTER);

    for (int i=0;i<10;i++)
    {
        addNumberButton(buttonPanel, String.valueOf(i));
    }

    addActionButton(buttonPanel, 1, "+");
    addActionButton(buttonPanel, 2, "-");
    addActionButton(buttonPanel, 3, "*");
    addActionButton(buttonPanel, 4, "/");
    addActionButton(buttonPanel, 5, "^2");

    JButton equalsButton = new JButton("=");
    equalsButton.setActionCommand("=");
    equalsButton.addActionListener(new ActionListener()
    {
        public void actionPerformed(ActionEvent event)
        {
            if (!numberCalc.getText().isEmpty())
            {
                int number = Integer.parseInt(numberCalc.getText()); 
                if (calcOperation == 1)
                {
                    int calculate = currentCalc  + number;
                    numberCalc.setText(Integer.toString(calculate));
                }
                else if (calcOperation == 2)
                {
                    int calculate = currentCalc  - number;
                    numberCalc.setText(Integer.toString(calculate));
                }
                else if (calcOperation == 3)
                {
                    int calculate = currentCalc  * number;
                    numberCalc.setText(Integer.toString(calculate));
                }
                else if (calcOperation == 4)
                {
                    int calculate = currentCalc  / number;
                    numberCalc.setText(Integer.toString(calculate));
                }
                else if (calcOperation == 5)
                {
                    int calculate = currentCalc  * currentCalc;
                    numberCalc.setText(Integer.toString(calculate));
                }
            }
        }
    });

    buttonPanel.add(equalsButton);
    guiFrame.setVisible(true);  
}

private void addNumberButton(Container parent, String name)
{
    JButton but = new JButton(name);
    but.setActionCommand(name);
    but.addActionListener(this);
    parent.add(but);
}

private void addActionButton(Container parent, int action, String text)
{
    JButton but = new JButton(text);
    but.setActionCommand(text);
    OperatorAction addAction = new OperatorAction(1);
    but.addActionListener(addAction);
    parent.add(but);
}

public void actionPerformed(ActionEvent event)
{
    String action = event.getActionCommand();

    numberCalc.setText(action);       
}

private class OperatorAction implements ActionListener
{
    private int operator;

    public OperatorAction(int operation)
    {
        operator = operation;
    }

    public void actionPerformed(ActionEvent event)
    {
        currentCalc = Integer.parseInt(numberCalc.getText()); 
        calcOperation = operator;
    }
}
}
\$\endgroup\$
11
\$\begingroup\$

Bug!

In your program, it's impossible to use any operator besides addition. As Code Review is not meant to be a code-bug-fixing site, I think you should try to fix this yourself. Hint: The bug is in your addActionButton method

Now, about your question

Yes, the template method is a good choice. Removing code duplication is always a good idea, which is I have to inform you about some other improvements you can make as well.

Some things you should think about first

  • Java coding convention is to put the { on the same line as before, not on it's own line.
  • It is good practice to use private final on as many class-variables ("fields") as possible.
  • It is an even better practice to reduce the scope of variables when possible, buttonPanel and guiFrame can be declared as local variables in the constructor.

Reducing duplication

  • numberCalc.setText(Integer.toString(calculate)); is done on every if-case, put it after all the if-else statements instead.
  • Instead of using several if-else, you can use a switch statement.
  • An enum is a better choice than using an int to store the current calcOperation.

Java 8

If you are not allowed or unable to use Java 8, skip this section :)

If you can use Java 8, there's a way to do this very smoothly. It may be a bit advanced if you are new to the Java language, but if you are willing to learn I suggest you check out IntBinaryOperator.

Your int calcOperation can be replaced with IntBinaryOperator operator;. Operations can be declared like this:

IntBinaryOperator add = (a, b) -> a + b;
IntBinaryOperator substract = (a, b) -> a - b;
IntBinaryOperator multiply = (a, b) -> a * b;
IntBinaryOperator divide = (a, b) -> a / b;

Your add action button can then become private void addActionButton(Container parent, IntBinaryOperator action, String text) {. This would greatly reduce code duplication for you, but as I said. This is only if you're using Java 8. If you're interested, add a comment and I'll explain more.


Regarding the user friendliness of your program, it's not layouted as a typical calculator. This is just a nitpick really. It would also be nice to use the keyboard for the program, but that's just a feature-request :)

\$\endgroup\$
  • \$\begingroup\$ I updated the code, I replaced the if else statements with switch, I created the JFrame guiFrame and JPanel buttonPanel within the class. I also fixed the bug, I already had it fixed in my compiler, it was probably an old version. { and } are now on the same line numberCalc.setText(Integer.toString(calculate)); now is outside the switch statement. I don't know what you meant with enum, or how to do it. My computer says it has Java 8 installed, but when I replace int calcOperation = 0; with IntBinaryOperator operator; it says it is an undefined variable. \$\endgroup\$ – TheWombatGuru Apr 1 '14 at 16:36
  • \$\begingroup\$ I also replaced the for loop to generate numbers for the number buttons with simply: addNumberButton("number"); otherwise i wouldn't be able to put the number in a logical order, or would I? \$\endgroup\$ – TheWombatGuru Apr 1 '14 at 20:30
  • 1
    \$\begingroup\$ @TheWombatGuru If you read the enum tutorial link in my post you should be able to use an enum. You can make one like public enum Calculation { ADDITION, SUBTRACTION, MULTIPLICATION, DIVISION; } and then you can declare a private Calculation activeCalculation; which you can set to one of those values. \$\endgroup\$ – Simon Forsberg Apr 1 '14 at 20:33
  • 1
    \$\begingroup\$ @TheWombatGuru For information on how to develop with Java 8, see wiki.eclipse.org/JDT/Eclipse_Java_8_Support_For_Kepler \$\endgroup\$ – Simon Forsberg Apr 1 '14 at 20:33
  • \$\begingroup\$ @Simon_André_Forsberg, I made a class for the enum, but I don't know how to implement it into the calculator, I changed the cases in the switch statement to the capital words, but I don't know how I can give actions to the words. \$\endgroup\$ – TheWombatGuru Apr 2 '14 at 19:55

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.