Java: where should I put anonymous listener logic code?

Question

we had a debate at work about what is the best practice for using listeners in java: whether listener logic should stay in the anonymous class, or it should be in a separate method, for example:

button.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent e) {
        // code here
    }
});

or

button.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent e) {
        buttonPressed();
    }
});

private void buttonPressed() {
    // code here
}

which is the recommended way in terms of readability and maintainability? I prefer to keep the code inside the listener and only if gets too large, make it an inner class. Here I assume that the code is not duplicated anywhere else.

Thank you.

Answer 1

My self-imposed rule is:

1. If the listener has more than 2 methods, create a named class.
2. If the listener spans more than 10 lines, create a named class.

Pretty simple, easy to follow and produces more or less readable code. But then I have to admit that I never even thought of what your example shows.

Answer 2

This question is partially answered here:

Is there a better practice for listeners

I also do not like the Anonymous way for two reasons:
1) you cannot easily re-use the code so you may find that you have duplicate code after a while 2) I find it breaks up the reading of the code (others disagree... personal taste). I think everyone would agree that if you are doing more than 5-10 lines that an anonymous inner class is not a good idea (I would say more than 2 is too much).

Answer 3

In my personal opinion, "it depends". If the listener is only to be added to a single component, is very simple and is an integral part of the GUI, then an anonymous inner class would work well. If the listener is complex, will be added to multiple components, will have its own separate state, then a separate stand alone class is better. In between is the private inner class. HTH.

Edit: My bad. I was answering a different question -- whether or not to use a separate stand-alone class for a listener. As for whether to keep anon inner class code inline vs. in a method, I agree with the other poster, it will depend on the size and complexity of the listener code.

Answer 4

If the method buttonPressed() will ever need to be accessed from anything aside from the anonymous inner class, then use a method. Otherwise simply placing the code within actionPerformed().

Answer 5

In my personal experience it is best to follow the MVC pattern. This way there is a separate class representing a model, which creates all relevant actions, action listeners etc. It is preferable that actions are represented as final class fields, which are instantiated in the constructor.

For example:

public class Model {
    private final Action buttonAction;
    ...

    public Model(final IController controller) {
       buttonAction = createButtonAction(controller);
       ...
    }

    private Action createButtonAction(final IController controller) {
        Action action = new Action("Action") {
            public void actionPerformed(final ActionEvent e) {
                // do the required action with long running ones on a separate Thread
                controller.run();
            }
        };
        action.set...// other initialisation such as icon etc
        ...
        return action;
    }
    ...

    public Action getButtonAction() {
        return buttonAction;
    }
}

The view is also represented by a separate class, which takes the model as its constructor parameter, where the actual button instantiation takes place. For example:

public class View extends JPanel {
    public View(final Model model) {
       ...
       JButton button = new JButton(model.getButtonAction();
       ...
    }
}

With this approach it is quite convenient to implement the logic of actionPerformed as part of the anonymous class since it has very little to be reused. All the logic is encapsulated into the controller, so the actions serve really as a wrapper around the controller call to be used as a model for a button.

Answer 6

Yes it depends highly on what you're trying to do. I think Anonymous inner classes have gotten a bad rap because of two myths. One is not being able to reuse anonymous code. And two, memory leaks. But these are easily fixed with a simple approach. Save a reference to the instance. For sharing code just create a reference to the anonymous inner class.

Action action = new AbstractAction("Open") {...};
JButton button = new JButton( action );
JMenuItem menuItem = new JMenuItem( action );
panel.getActionMap().put("openFile", action );

Now you can reuse that action across multiple components. For the later problem of memory leaks you can use that reference to unregister it, or the second and simpler option is WeakListeners. WeakListeners have the advantage of not needing to managed after their creation as much.

As for style I find Anonymous listeners to be quite handy, and in some cases easier to read when dealing with threading in Swing because it keeps your code in one method (invokeLater, executeInBackground, etc). When you anon listener delegates to an instance method I think it separates the code where you can't read what happened prior to the listener and the logic associated with the listener in one screen. They tend to get separated, and it's harder to follow.

Something to be aware of is if you use ActionMaps most of the memory leaks will go away associated with keyboard listening. Unfortunately things like focus listeners or listeners that are registered with central systems are still a problem. (Again WeakListeners are great here). And you already have a place to keep the action with every component so no need to create extra instance variables to hold it. If you need to reuse across two components (say in a menu bar and in a control) create a separate class.