Using actionPerformed() to call different methods per radio button selection

Question

My Java is very rusty and I have been unlucky finding a better way to accomplish this task. I have two different methods which get called based on which radio button is selected. Is there a cleaner way to store/determine which radio button is currently selected in order to call the appropriate method? Here is the segment I am trying to streamline. (Full source code below)

public void actionPerformed(ActionEvent e){
    if(e.getSource() == iterativeRadio){
        calcMethod = "iterative";
    }else if(e.getSource() == recursiveRadio){
        calcMethod = "recursive";
    }else if(calcMethod == null){
        resultBox.setText("Select a calculation method");
    }else{
        try{
            int n = Integer.parseInt(inputBox.getText());
            if (e.getSource() == calcButton){
                if(calcMethod == "iterative"){
                    resultBox.setText(String.valueOf(Sequence.computeIterative(n)));
                    efficiencyBox.setText(String.valueOf(Sequence.getEfficiency()));
                }else if(calcMethod == "recursive"){
                    resultBox.setText(String.valueOf(Sequence.computeRecursive(n)));
                    efficiencyBox.setText(String.valueOf(Sequence.getEfficiency()));
                }
            }
        }catch (NumberFormatException e2){
            resultBox.setText("Invalid entry");
        }catch (ArithmeticException e3){
            resultBox.setText(e3.getMessage());
        }catch (Exception e4){
            resultBox.setText("Something bad happened");
        }
    }
}

Sequence.java:

/**
 * @author Matthew Miller
 * Sequence utility class
 */

public final class Sequence {
    private static int efficiency;

    public static int getEfficiency(){
        return efficiency;
    }

    public static int computeIterative (int n){
        efficiency = 0;
        return iterativeWorker(0,1,n);
    }

    private static int iterativeWorker(int onePrev, int curr, int n){
        if (n == 0){
            return 0;
        }else{
            int twoPrev;
            for(int i = curr; i < n; i++){
                efficiency++;
                twoPrev = onePrev;
                onePrev = curr;
                curr = Math.addExact(Math.multiplyExact(2, onePrev), twoPrev);
            }
            return curr;
        }
    }

    public static int computeRecursive (int n){
        efficiency = 0;
        return recursiveWorker(n);
    }

    private static int recursiveWorker(int n){
        efficiency++;
        if (n == 0){
            return 0;
        }else if (n == 1){
            return 1;
        }else{
            return Math.addExact(Math.multiplyExact(2, recursiveWorker(n - 1)), recursiveWorker(n - 2));
        }
    }
}

SequenceCalc.java:

import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;
import java.io.BufferedWriter;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.Writer;

import javax.swing.BorderFactory;
import javax.swing.ButtonGroup;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JRadioButton;
import javax.swing.JTextField;

/**
 * @author Matthew Miller
 * SequenceCalc/GUI class
 */

public class SequenceCalc extends JPanel implements ActionListener {
    private static final long serialVersionUID = -5799806787577444436L;

    private String calcMethod = "iterative";

    private final JButton calcButton = new JButton("Calculate");
    private final JRadioButton iterativeRadio = new JRadioButton("Iterative",true);
    private final JRadioButton recursiveRadio = new JRadioButton("Recursive");
    private final ButtonGroup radioGroup = new ButtonGroup();
    private final JTextField inputBox = new JTextField();
    private final JTextField resultBox = new JTextField();
    private final JTextField efficiencyBox = new JTextField();

    public SequenceCalc(){
        iterativeRadio.addActionListener(this);
        recursiveRadio.addActionListener(this);
        calcButton.addActionListener(this);

        resultBox.setEditable(false);
        efficiencyBox.setEditable(false);

        radioGroup.add(iterativeRadio);
        radioGroup.add(recursiveRadio);

        JFrame frame = new JFrame("Project 3");
        JPanel mainPanel = new JPanel(new GridLayout(4,1,1,1));
        mainPanel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
        JPanel nRow = new JPanel(new GridLayout(1,2,1,1));
        nRow.add(iterativeRadio);
        nRow.add(recursiveRadio);
        mainPanel.add(nRow);
        nRow = new JPanel(new GridLayout(1,2,1,1));
        nRow.add(inputBox);
        nRow.add(calcButton);
        mainPanel.add(nRow);
        nRow = new JPanel(new GridLayout(1,2,1,1));
        nRow.add(new JLabel("Result:"));
        nRow.add(resultBox);
        mainPanel.add(nRow);
        nRow = new JPanel(new GridLayout(1,2,1,1));
        nRow.add(new JLabel("Efficiency:"));
        nRow.add(efficiencyBox);
        mainPanel.add(nRow);
        frame.add(mainPanel);
        frame.pack();
        frame.setVisible(true);
        frame.setDefaultCloseOperation(JFrame.DO_NOTHING_ON_CLOSE);
        frame.addWindowListener(new WindowAdapter(){
                @Override
                public void windowClosing(WindowEvent e){
                    writeFile(generateReport(0,10));
                    frame.dispose();
                }
            });
    }

    @Override
    public void actionPerformed(ActionEvent e){
        if(e.getSource() == iterativeRadio){
            calcMethod = "iterative";
        }else if(e.getSource() == recursiveRadio){
            calcMethod = "recursive";
        }else if(calcMethod == null){
            resultBox.setText("Select a calculation method");
        }else{
            try{
                int n = Integer.parseInt(inputBox.getText());
                if (e.getSource() == calcButton){
                    if(calcMethod == "iterative"){
                        resultBox.setText(String.valueOf(Sequence.computeIterative(n)));
                        efficiencyBox.setText(String.valueOf(Sequence.getEfficiency()));
                    }else if(calcMethod == "recursive"){
                        resultBox.setText(String.valueOf(Sequence.computeRecursive(n)));
                        efficiencyBox.setText(String.valueOf(Sequence.getEfficiency()));
                    }
                }
            }catch (NumberFormatException e2){
                resultBox.setText("Invalid entry");
            }catch (ArithmeticException e3){
                resultBox.setText(e3.getMessage());
            }catch (Exception e4){
                resultBox.setText("Something bad happened");
            }
        }
    }

    public static String generateReport(int start, int end){
        StringBuilder sb = new StringBuilder();
        sb.append("Value of n,").append("Iterative,").append("Recursive\n");
        for (int i=start;i<=end;i++){
            //sb.append(i).append(",");
            //Sequence.computeIterative(i);
            sb.append(Sequence.computeIterative(i)).append(",");
            sb.append(Sequence.getEfficiency()).append(",");
            Sequence.computeRecursive(i);
            sb.append(Sequence.getEfficiency()).append("\n");
        }
        return sb.toString();
    }

    private static void writeFile(String content){
        try{
            Writer fd = new BufferedWriter(new OutputStreamWriter(
                    new FileOutputStream("efficiency.csv"), "utf-8"));
            fd.write(content);
            fd.close();
        }catch(IOException e){
            System.out.println(e.getMessage());
        }
    }

    public static void main(String[] args){
        new SequenceCalc();
    }
}

Answer 1

Indeed, the conditional branching in actionPerformed(ActionEvent) is quite complex and prone to bugs. This implementation does too many things at the same time: sets the calcMethod value, determines which calculation method to use and executes the calculation. These concerns should be separated. Each GUI element should trigger only the changes that are relevant to its responsibility. JRadioButtons, when clicked, should define the calculation method to use; calcButton should trigger the calculation.

Without significant changes to the design of SequenceCalc class, the separation of these concerns can be achieved as follows.

In order to extract the selection of the calculation method, we will use a simple interface

interface CalculationRunner {

  int runCalculation(int times);

}

It has two ad-hoc anonymous implementations:

private static final CalculationRunner RUNNER_ITERATIVE = new CalculationRunner() {

  @Override
  public int runCalculation(int times) {
    return Sequence.computeIterative(times);
  }
};

private static final CalculationRunner RUNNER_RECURSIVE = new CalculationRunner() {

  @Override
  public int runCalculation(int times) {
    return Sequence.computeRecursive(times);
  }
};

The reference to the currently chosen method will be stored in a dedicated field:

// let's say that it will be initialiazed to "iterative", since the 
// iterativeRadio is selected by default
private CalculationRunner runner = RUNNER_ITERATIVE;

We do not need String calcMethod field in this class any more.

The events handling will be separate for each element of the GUI. In the constructor:

iterativeRadio.addActionListener(new ActionListener() {

  @Override
  public void actionPerformed(ActionEvent e) {
    SequenceCalc.this.runner = RUNNER_ITERATIVE;
  }
});

recursiveRadio.addActionListener(new ActionListener() {

  @Override
  public void actionPerformed(ActionEvent e) {
    SequenceCalc.this.runner = RUNNER_RECURSIVE;
  }
});

The main logic of the action will now be reduced to the following:

calcButton.addActionListener(new ActionListener() {

  @Override
  public void actionPerformed(ActionEvent e) {
    try {
      int n = Integer.parseInt(inputBox.getText());
      final int result = SequenceCalc.this.runner.runCalculation(n);
      resultBox.setText(String.valueOf(result));
      efficiencyBox.setText(String.valueOf(Sequence.getEfficiency()));
    }
    catch (NumberFormatException e2) {
      resultBox.setText("Invalid entry");
    }
    catch (ArithmeticException e3) {
      resultBox.setText(e3.getMessage());
    }
  }
});

And there is no more need to implement ActionListener for SequenceCalc, actionPerformed should be deleted from it.

Other Stuff

1. The solution above is just a quick ad-hoc tip. Personally, from the point of view of the style, I don't like in-line anonymous implementations. They can be extracted in dedicated entities.

2. I doubt a lot about the static methods and static efficiency member in Sequence. This should be a normal object without statics.

3. The constructor is too long. It should be split based on semantic parts (GUI elements, listeners, frame config etc).

4. catch (Exception e4) is ugly, I removed it from the main action processing.

Answer 2

if(calcMethod == "iterative") will work in Java because of String caching, but in general you should be using if("iterative".equals(calcMethod))).

You could use anonymous classes to set your calcMethod member:

iterativeRadio.addActionListener(new ActionListener() {
    @Override public void actionPerformed(ActionEvent e) {
        calcMethod = "iterative";
    }
});

which will ease up the code in you "main" actionPerformed() method.

To take it one step further, if you have a fixed number of calculation methods, you'd be happier using Java enums. It basically is a fixed set of class instances (not only named numbers):

public enum Sequence {

    ITERATIVE {
        @Override
        public int compute(int n) {
            // TODO do the computation and compute efficiency
            return 0;
        }
    },

    RECURSIVE {
        @Override
        public int compute(int n) {
            // TODO do the computation and compute efficiency
            return 0;
        }
    };

    // Regular class definition with members and methods

    private int efficiency;

    public int getEfficency() {
        return efficiency;
    }

    // Abstract method to be implemented by all enum instances
    public abstract int compute(int n);

}

You can then map one or the other to a client property on your radio button:

iterativeRadio.putClientProperty("seq", Sequence.ITERATIVE);
recursiveRadio.putClientProperty("seq", Sequence.RECURSIVE);

and transform your String calcMethod into Sequence calcMethod. Then use the same ActionListener on all your radio buttons:

ActionListener al = new ActionListener() {
    @Override public void actionPerformed(ActionEvent e) {
        calcMethod = (Sequence)((JRadioButton)e.getSource()).getCLientProperty("seq");
    }
};
iterativeRadio.addActionListener(al);
recursiveRadio.addActionListener(al);

Your "main" actionPerformed() would then boil down to:

public void actionPerformed(ActionEvent e){
    try{
        int n = Integer.parseInt(inputBox.getText());
        resultBox.setText(String.valueOf(calcMethod.compute(n)));
        efficiencyBox.setText(String.valueOf(calcMethod.getEfficiency()));
    }catch (NumberFormatException e2){
        resultBox.setText("Invalid entry");
    }catch (ArithmeticException e3){
        resultBox.setText(e3.getMessage());
    }catch (Exception e4){
        resultBox.setText("Something bad happened");
    }
}

Neat, isn't it? :)