a challenge; contest if you will

[Randall Twede]

a lot of you think the best way to handle action events is a separate anonymous inner class for each button. i challenge this. since java 7 i can have a switch statement that uses Strings. i think my code is shorter, more readable and more maintainable. i also contend it is just as object oriented. as for the ButtonListener class or the actionPerformed() method being too long, it is only two local variables and a switch statement.

so here is the challenge. i will post my code. if someone wants to rewrite it to use anonymous inner classes, we can let JavaRanch decide which they prefer.

import java.awt.*;  
import java.awt.event.*;  
import javax.swing.*;  
      
class Calculator  
{  
    private JFrame frame = new JFrame();  
    private JTextField display = new JTextField();  
    private JPanel buttonPanel = new JPanel(new BorderLayout(2,2));  
    private JPanel topPanel = new JPanel(new GridLayout(1,3,5,15));  
    private JPanel centerPanel = new JPanel(new GridLayout(4,5,5,5));  
    private JPanel westPanel = new JPanel(new GridLayout(5,1,0,4));  
    private JLabel memoryFlag = new JLabel(); 
    private JButton[] buttons = new JButton[27];  
    private String[] btnText = {"Backspace","CE","C","MC","MR","MS","M+","7","8","9","/","sqrt","4","5","6","*","%","1","2","3","-","1/x","0","+/-",".","+","="};
	private ButtonListener listener = new ButtonListener();
	private double operandA;
	private double operandB;
	private String operator;
	private Double result;
	private boolean isNegative = false;
	private String memory = "";
	
    public Calculator()  
    {  
        initComponents();  
        frame.setSize(265, 265);  
        frame.setLocationRelativeTo(null);  
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);  
        frame.setTitle("Calculator");  
        frame.getContentPane().setLayout(new BorderLayout(2,2));  
        frame.add(display, "North");  
        frame.add(westPanel, "West");  
        frame.add(buttonPanel, "Center");  
        frame.setVisible(true);  
    }  
    private void initComponents()  
    {  
        for(int x = 0, y = buttons.length; x < y; x++)  
        {  
            buttons[x] = new JButton(btnText[x]);  
            buttons[x].setPreferredSize(new Dimension(36,36));  
            buttons[x].setMargin(new Insets(0,0,0,0));  
            buttons[x].setFont(buttons[x].getFont().deriveFont(11f));
			buttons[x].addActionListener(listener);
        }  
        display.setMargin(new Insets(5,0,5,0));  
        memoryFlag.setBorder(BorderFactory.createBevelBorder(javax.swing.border.BevelBorder.LOWERED));  
        display.setHorizontalAlignment(JTextField.TRAILING);  
        for(int x = 0; x < 3; x++) topPanel.add(buttons[x]);  
        for(int x = 7, y = buttons.length; x < y; x++) centerPanel.add(buttons[x]);  
        westPanel.add(memoryFlag);  
        for(int x = 3; x < 7; x++) westPanel.add(buttons[x]);  
        buttonPanel.add(topPanel, "North");  
        buttonPanel.add(centerPanel, "Center");  
    }  
	private void binary()
	{
		switch(operator)
		{
			case "+":
				result = new Double(operandA + operandB);
				display.setText(result.toString());
				break;
			case "-":
				result = new Double(operandA - operandB);
				display.setText(result.toString());
				break;
			case "*":
				result = new Double(operandA * operandB);
				display.setText(result.toString());
				break;
			case "/":
				result = new Double(operandA / operandB);
				display.setText(result.toString());
				break;
			case "%":
				result = new Double(operandA % operandB);
				display.setText(result.toString());
				break;
		}
	}
	
	class ButtonListener implements ActionListener
	{
		public void actionPerformed(ActionEvent e)
		{
			String button = e.getActionCommand();
			String text = display.getText();
			switch(button)
			{
				//numeric
				case "0":
				case "1":
				case "2":
				case "3":
				case "4":
				case "5":
				case "6":
				case "7":
				case "8":
				case "9":
				case ".":
					display.setText(text + button);
					break;
				//binary
				case "+":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						operator = "+";
						display.setText("");
					}
					break;
				case "-":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						operator = "-";
						display.setText("");
					}
					break;
				case "*":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						operator = "*";
						display.setText("");
					}
					break;
				case "/":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						operator = "/";
						display.setText("");
					}
					break;
				case "%":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						operator = "%";
						display.setText("");
					}
					break;
				case "=":
					if(!text.equals(""))	//prevent exception
					{
						operandB = Double.parseDouble(text);
						binary();
					}
					break;
				//unary
				case "1/x":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						result = new Double(1 / operandA);
						display.setText(result.toString());
					}
					break;
				case "sqrt":
					if(!text.equals(""))	//prevent exception
					{
						operandA = Double.parseDouble(text);
						result = new Double(Math.sqrt(operandA));
						display.setText(result.toString());
					}
					break;
				//memory
				case "MC":
					memory = "";
					memoryFlag.setText("");
					break;
				case "MR":
					display.setText(memory);
					break;
				case "MS":
					memory = text;
					memoryFlag.setText("M");
					display.setText("");
					break;
				case "M+":
					if(!text.equals(""))	//prevent exception
					{
						Double sum = new Double(Double.parseDouble(memory) + Double.parseDouble(text));
						memory = sum.toString();
						display.setText("");
					}
					break;
				//misc
				case "C":
					operandA = 0;
					operator = "";
				case "CE":
					display.setText("");
					break;
				case "Backspace":
					if(!text.equals(""))	//prevent exception
					{
						display.setText(text.substring(0, (text.length() - 1)));
					}
					break;
				case "+/-":
					if(!text.equals(""))	//prevent exception
					{
						if(isNegative)
						{
							display.setText(text.substring(1, text.length()));
						}
						else
						{
							display.setText("-" + text);
						}
						isNegative = isNegative ? false : true;
					}
					break;
			}	//end switch
		}		//end actonPerformed()			
	}			//end ButtonListener
	
    // create and display an instance of Calculator
    public static void main(String args[])
    {  
        SwingUtilities.invokeLater(new Runnable()
		{  
            public void run()
			{  
				new Calculator();  
      	    }  
		});
    }
}

[Rob Spoor]

I definitely wouldn't use anonymous inner classes for each buttons here. A lot of the buttons share functionality, so I'd definitely use that. A quick untested example:

// called as new NumberListener(1), new NumberListener(2), etc
class NumberListener implements ActionListener
{
    private int num;

    public NumberListener(int number)
    {
        num = number;
    }

    public void actionPerformed(ActionEvent e)
    {
        display.setText(text + num);
    }
}

// called as new BinaryOperatorListener("+"), new BinaryOperatorListener("-"), etc
class BinaryOperatorListener implements ActionListener
{
    private String op;

    public BinaryOperatorListener(String operator)
    {
        op = operator;
    }

    public void actionPerformed(ActionEvent e)
    {
        if (!text.equals(""))
        {
            operatorA = Double.parseDouble(text);
            operator = op;
            display.setText("");
        }
    }
}

Unfortunately, 1/x and sqrt are the only ones remaining that actually share code, although you'd need an abstract method for that.

Some hints about your existing code:
1) Change isNegative = isNegative ? false : true; into isNegative = !isNegative;
2) If you decide to stick with the large switch statement, you can group "+", "-", "*", "%" and "/" like this:

                //binary
                case "+":
                case "-":
                case "*":
                case "/":
                case "%":
                    if(!text.equals(""))    //prevent exception
                    {
                        operandA = Double.parseDouble(text);
                        operator = button;
                        display.setText("");
                    }
                    break;

[Randall Twede]

i take that as 1/0 in favor of my code.
thanks rob for the help optimizing my code.
i changed it because i thought the binary() method and the operator variable should be in the inner class not the outer class.
i will also use Rob's changes. i will post the new when i can get online with my laptop. im using the state's computer now. the challenge still stands.

[Matthew Brown]

Randall Twede wrote:i take that as 1/0 in favor of my code.
thanks rob for the help optimizing my code.
i changed it because i thought the binary() method and the operator variable should be in the inner class not the outer class.
i will also use Rob's changes. i will post the new when i can get online with my laptop. im using the state's computer now. the challenge still stands.

How do you interpret Rob posting an alternative that doesn't have all the different responses in a single event handler - that, crucially, follows the important principle of cohesion (unlike your code - which is why I'd say it's not as object-oriented) - as 1/0 in your favour?

[Randall Twede]

it sounded like he agreed.

i'll rewrite it taking rob's observations in mind

[Paul Clapham]

Randall Twede wrote:it sounded like he agreed.

Yes, it did. And yet if you look at the code that Rob posted after he "agreed", there are two ActionListeners there.

You're making this into a contest between "15 ActionListeners" and "1 ActionListener". In my opinion (like what Rob posted) neither is the winner. The right answer is a number around 3 or 4.

[Matthew Brown]

Paul Clapham wrote:You're making this into a contest between "15 ActionListeners" and "1 ActionListener". In my opinion (like what Rob posted) neither is the winner. The right answer is a number around 3 or 4.

3 or 4 ActionListener classes, but there would be about 15 instances in Rob's version.

The point I'd come back to is that ActionListeners, like any other class, should be cohesive. They should do one thing. A single class handling 10 events that differ only in the number they print is great - just parameterise the class. I don't think anyone is arguing for separate anonymous listeners in that case, as you'd have needless repetion (Don't Repeat Yourself). I think the original challenge is misleading.

[Randall Twede]

cohesion...i fail to see how my code is not cohesive. perhaps you could point it out to me. i really should read code people post. Rob's code addresses the one thing i didn't like about mine. the repeated code "if (!text.equals(""))"

ActionListeners, like any other class, should be cohesive. They should do one thing.

i argue that it does do one thing. it handles the action events

i might change it to two event handlers though to get rid of the repeated if's

the only other "problem" i see is, from a MVC point of view, i am mixing the model, view, and controller in my event handler. however it is a very small program.