Nifty method at adding an actionListener, but is it better or worse?

[stevenfriz]

I'm a code organization freak and I've been dwelling over a way to clean up 23 lines of code that repeat "object.addActionListener(this)" with object being a JMenuItem or something. I felt like that 23 lines of code can be shortened a few lines and I've done it! It's nothing special, but would the below code be faster or slower when processing? I assume slower.

		for (int i = 0; i < menuBar.getComponentCount(); i++) {
			if (menuBar.getComponent(i) instanceof JMenu) {
				for (int x = 0; x < menuBar.getMenu(i).getMenuComponentCount(); x++) {
					if (menuBar.getMenu(i).getMenuComponent(x) instanceof Separator) {
						continue;
					} else {
						JMenuItem get = (JMenuItem) menuBar.getMenu(i).getMenuComponent(x);
						setListener(get);
					}
				}
			}
		}

Assume menuBar is a JMenuBar. The first loop gets the Menu's of the Menubar, and then that first if statement tests if it is a JMenu or not. If true, it heads on over to the next for loop which get's a menu at I, the control variable of the first loop. The JMenu at i, returns the component count of that menu. If the component found in the menubar is an instanceof Separator it hits continue; . If false, It gets the MenuItem and set's the listener through a method. The instanceof Separator is there because I have separators in my Menu's.

Any thoughts? I hate having lines of code that repeat the same thing, so I cleaned it up with this. Is this faster or slower? Better or for the worse?

[Fubarable]

Speed of this code is irrelevant since it is a trivially small piece of code that isn't even called when the GUI is running but rather runs only once when the GUI is being set up. Once the the GUI is running, how you add listeners to menus or buttons doesn't matter at all.

I think though that there are other potential problems here. The speed that matters most in this situation is the time it takes you to debug, maintain and upgrade your application, and having all of your menu items use the same listener is a potential problem since you may be forcing that listener object to do too many unrelated things, to make it disobey the Single Responsibility Principle of object oriented programming.

[DarrylBurke]

addActionListener(this) is evil.

db

[stevenfriz]

addActionListener(this) is evil.

db

How come?

[stevenfriz]

So say, If I create a class for every GUI component of a TextEditor (JMenubar, JTabbedPane, JToolBar, etc) then had a main class construct the text editor from every class, would that be bad? Or should I just create the whole GUI in one class.

[cselic]

How come?

Well its better for you to use something like this:

addActionListener(new ActionListener() {
			@Override
			public void actionPerformed(ActionEvent e) {
				// your code goes here
			}
		});

or maybe like this:

addActionListener(new ActionListenerThatIWantToWrite());

and then inside of your existing class make new private class that implements action listener. It should be something like this:

private class ActionListenerThatIWantToWrite implements ActionListener {

	@Override
	public void actionPerformed(ActionEvent e) {
		// your code goes here
	}
		
}

[stevenfriz]

I have used your first method many times, but it creates Anonymous classes. When it comes to a menubar that has 20+ items, that creates 20+ anonymous classes in the compiled jar file, which to me is excessive and annoying. I'm starting to use your second method, with the exception I didn't know you could make classes private.