Refactoring multiple actionListeners

Question

I am currently working on some Java code that has a lot of ActionListeners defined in it (one for each JButton) and there are around 60 buttons. These are all defined as anonymous inner classes in the JButton.addActionListener method. I have been thinking of ways to refactor this to make the code look neater as this is making it looked very cluttered. I thought about possibly taking the listeners into a separate class that essentially has a load of static methods each returning a listener. This will mean that the code will look something like addActionListener(GetActionListener.addActionListener()). Whilst this will make it neater I feel that it is not really an elegant solution. I thought also a static final map holding KV pairs with listener name to the listener itself. Again however this still does not seem like a very elegant solution. I was wondering if anyone had any ideas? I should also say that all the actionListeners are pretty different.

Answer 1

Not a duplication of this question (which was not a duplicate of the question it answered... wow) but the answer should apply.

If your inner classes are doing more than just calling a method inside of the outer class then you are doing it "wrong" (to my definition of "right"). In the posted code the calls to increment() and decrement() are the "right" way to do it. Refactoring the code have the listeners forward the method call to the outer class is a good place to start to make the code more manageable.

That being said... 60 buttons on a UI?! Really! Ouch! Are they all on one screen or is it done with tabs or something? (if it is tabs or something I have more to offer in the answer).

Answer 2

I would suggest not to directly add actions using ActionListener. If you do this way it becomes non-reusable. Instead wrap your actions in javax.swing.Action class. So that you can reuse the action wherever you want. For e.g now you can use the same action for say a menu shortcut of a Copy action and the copy button in toolbar. Basically the idea is not to directly couple runnable actions with GUI elements.

Now coming to your question. I would make a repository of actions in a class called, say, ActionRepsoitory with a public method public Action getAction(String). Each of your action would be identified by a String constant which you use to retrieve the action from the repository. Typically that string would be the actionCommand for the element. How you manage the actions in the ActionRepository, via a HasMap or whatever, is completely dependent on you.

This is how its doen in most proffesional code, AFAIK.

Answer 3

You could make a special Subclass of ActionListener that uses reflection to call a given method name, then you can implement all your 60 actions as normal methods.

package com.example;

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;

public class MethodAsActionListener implements ActionListener {

    private Object receiver;
    private Method method;

    public MethodAsActionListener(Object receiver, String name) {
        this.receiver = receiver;
        try {
            this.method = receiver.getClass().getDeclaredMethod(name);
        } catch (SecurityException e) {
           throw new RuntimeException(e);
        } catch (NoSuchMethodException e) {
            throw new RuntimeException(e);
        }
    }

    @Override
    public void actionPerformed(ActionEvent event) {
        try {
            method.invoke(receiver);
        } catch (IllegalArgumentException e) {
            throw new RuntimeException(e);
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        } catch (InvocationTargetException e) {
            throw new RuntimeException(e);
        }
    }

}

and then if you a method call to your class

private call(String name) {
    return new MethodAsActionListener(this, name);
}

then you can add your actions as follows

button1.addActionListener(call("methodName1"));
button2.addActionListener(call("methodName2"));
button3.addActionListener(call("methodName3"));
button4.addActionListener(call("methodName4"));
button5.addActionListener(call("methodName5"));

if one of these methods is missing, the program will fail when the UI is built (since we lookup the method when the action listener is created). Which is not as nice as at compile time, but still better than totally late-bound when the action is triggered.

Answer 4

I would recommend something like what you suggested--create a single listener class that you subscribe to all the events. You PROBABLY want to use a different instance of the class for each event though, telling the instance (in the constructor) in general what to do with this specific event.

The advantage of this is that you can then start factoring the code inside the listeners together into fewer methods because It's usually pretty similar. Sometimes you can get it into a single method.

One trick I've used for a "Pure dispatch" situation for menu creation was to specify the menu, the structure of the menu and the method each menu item links to in data. Needs a little reflection but it works.

In fact--let me look.

Yeah, I kept the classes in a google doc :) The data was specified like this:

final static String[] menus = { "File:*", "Save:save", "Load:load", "(", "Print:-", "Preview:preview", ")", "Quit:quit" };

It just parsed this. File becomes a top level item because of the start, save will call your "Save" method, load will call your "Load" method, Print is a sub-menu (hence the parens), with preview underneath it and print is not bound to anything.

This string can create and bind an entire menu with one call.

Here's my source code if you want to play with it.

The "TestMenu" class at the top is a testing class demonstrating how to use the buildMenus method.

This was done quite a few years ago, I might do it differently now, but it works. I'm not sure I like it actually generating the menu, and I think I'd make the string parser use a single string instead of breaking it down into strings for each item--should be easy to ensure each item is whitespace separated...

A better API might be a bind method like this:

bind(this, new JButton("Save"), "save", this);

where pressing the save button would cause the save method to be called on this (or whatever other object you passed in). You could even make the "save" parameter optional and just use the JButton.getText().toLower() as the method to call if no parameter exists (I guess that's convention before configuration)

I didn't do it this way with the menu because I also wanted to abstract out the menu creation and menu relationships into my data.

Note that coding this way is an awesome way to get your MVC separation in Java--all your controller code can be removed from your view.