Switching on a string/implementing button actions

Question

Full disclaimer: I'm a CS student, and this question is related to a recently assigned Java program for Object-Oriented Programming. Although we've done some console stuff, this is the first time we've worked with a GUI and Swing or Awt. We were given some code that created a window with some text and a button that rotated through different colors for the text. We were then asked to modify the program to create radio buttons for the colors instead—this was also intended to give us practice researching an API. I've already handed in my assignment and received permission from my instructor to post my code here.

What's the best way to implement button actions in Java? After some fiddling around, I created the buttons like this:

class HelloComponent3 extends JComponent
    implements MouseMotionListener, ActionListener
{
    int messageX = 75, messageY= 175;

    String theMessage;
    String redString = "red", blueString = "blue", greenString = "green";
    String magentaString = "magenta", blackString = "black", resetString = "reset";

    JButton resetButton;
    JRadioButton redButton, blueButton, greenButton, magentaButton, blackButton;
    ButtonGroup colorButtons;

    public HelloComponent3(String message) {

    theMessage = message;

    //intialize the reset button
    resetButton = new JButton("Reset");
    resetButton.setActionCommand(resetString);
    resetButton.addActionListener(this);

    //intialize our radio buttons with actions and labels
    redButton = new JRadioButton("Red");
    redButton.setActionCommand(redString);
    ...

And added action listeners...

redButton.addActionListener(this);
blueButton.addActionListener(this);
...

A stub was already created for the actionPerformed method to give us an idea on how to use it, but since there was only a single button in the template, it wasn't clear how to implement multiple buttons. I tried switching on a String, but quickly realized that, since a String isn't a primitive type, I couldn't use it for a switch statement. I could have improvised with an if-else chain, but this is what I came up with instead. It seems far from elegant, and there must be a better way. If there is, what is it? Is there a way to switch on a string? Or choose an action in a more scaleable fashion?

public void actionPerformed(ActionEvent e){

    if (e.getActionCommand().equals(resetString)) {
     messageX = 75; messageY = 175;
     setForeground(Color.black);
     blackButton.setSelected(true);
     repaint();
     return;
    }

    if ( e.getActionCommand().equals(redString) ) {
     setForeground(Color.red);
     repaint();
     return;
    }

    if ( e.getActionCommand().equals(blueString) ) {
     setForeground(Color.blue);
     repaint();
     return;
    }

    if ( e.getActionCommand().equals(greenString) ) {
     setForeground(Color.green);
     repaint();
     return;
    }

    if ( e.getActionCommand().equals(magentaString) ) {
     setForeground(Color.magenta);
     repaint();
     return;
    }

    if ( e.getActionCommand().equals(blackString) ) {
     setForeground(Color.black);
     repaint();
     return;
    }
}

Answer 1

Instead of writing this:

resetButton.addActionListener(this);

You could also write this:

resetButton.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent evt) {
        resetButtonActionPerformed(evt);
    }
});

And instead of writing one big actionPerformed() for all actions, you can (and then have to) write this:

public void resetButtonActionPerformed(ActionEvent evt) {
    messageX = 75; messageY = 175;
    setForeground(Color.black);
    blackButton.setSelected(true);
    repaint();
}

I don't know if this is the most elegant solution, but at least you no longer have that big if construct.

Answer 2

Two alternate approaches:

1. Create a new class that implements the Action interface and has a Color field and an actionPerformed method that sets the color
2. Mantain a HashMap from command names to Color instances and look up the command name in the map

Answer 3

Ergh. Don't implement masses of unrelated interfaces in one mega class. Instead, use anoymous inner classes. They are a bit verbose, but are what you want. Use one for each event, then you wont need big if-else chain. I suggest keeping enough code within the inner class to decode the event and call methods that make sense to the target objects. Further, you can parameterise your inner classes. You will probably find you don't need to keep references to the actual widgets around.

In your example you seem to be using a JComponent as a JPanel. There's not much difference, but use JPanel for collecting a block of widgets. Further there is unlikely any need to subclass it, so don't.

So for instance:

   addColorButton("Green" , Color.GREEN );
   addColorButton("Red"   , Color.RED   );
   addColorButton("Yellow", Color.YELLOW);
   addColorButton("Blue"  , Color.BLUE  );
   ...

private void addColorButton(String label, Color color) {
    JRadioButton button = new JRadioButton(label);
    button.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent event) {
            target.setForeground(color);
            target.repaint();
        } 
    });
    colorGroup.add(button);
    panel.add(button);
}

Answer 4

One decent enough approach is to declare an enum whose elements match your strings and switch on valueOf(str) (the linked example shows how to do this with a fair amount of safety).

The reason to avoid anonymous inner classes is probably because the class hasn't had that construct (yet), even though that might be the best solution.

Answer 5

As suggested already, you can use anonymous inner classes to implement the ActionListener interface. As an alternative, you don't have to use anonymous inner classes, but you can use a simple nested class instead:

resetButton = new JButton(new ResetAction());
redButton = new JButton(new ColorAction("Red", Color.red));

and then...

private class ResetAction extends AbstractAction {
    public ResetAction() {
        super("Reset");
    }

    public void actionPerformed(ActionEvent e) {
        messageX = 75; messageY = 175;
        setForeground(Color.black);
        blackButton.setSelected(true);
        repaint();
    }
}

private class ResetAction extends AbstractAction {
    private Color color;

    public ColorAction(String title, Color color) {
        super(title);
        this.color = color;
    }

    public void actionPerformed(ActionEvent e) {
        setForeground(color);
        repaint();
    }
}

For why this approach - or any approach involving inner classes - is better than implementing ActionListener in the outer class see "Design Patterns":

"Favor 'object composition' over 'class inheritance'." (Gang of Four 1995:20)

Choosing between anonymous inner classes and these named inner classes is a largely a matter of style, but I think this version is easier to understand, and clearer when there are lots of actions.