3
\$\begingroup\$

I'm rewriting an old Swing project that I did a few months ago, a fairly simple copy-cat of MS paint. I've been doing a pretty good job of keeping the code maintainable and organized, but I've hit a wall on one class. All I need is some general tips on organization on this one class.

The way I have my program set up is that each component of my painting program (buttons for colors, eraser button, JPanel to paint on) is actually a relative (extended from) of another swing component class (for instance PaintPanel, the panel you paint on, extends JPanel, or colorButton is an extension of JButton). This just allows me to keep things encapsulated and organized.

So, I needed a JFileMenu for the top of the program in order to save, open files, edit, etc. I created a new Class called PaintMenuBar, extended it from JMenuBar, and started creating the JMenus and JMenuItems. Now, each JMenuItem obviously needs an ActionListener, so I just gave each one a different ActionListener. I'm skeptical about this because I know it can be kind of disorganized looking.

Basically, I just want somebody to let me know if this practice is all right, or if there is a better alternative that doesn't involve me making 20 new classes.

Here's the entire class:

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;

public class PaintMenuBar extends JMenuBar
{
    private static final long serialVersionUID = 1L;

    JMenu fileMenu;
    JMenu editMenu;
    JMenu aboutMenu;

    public PaintMenuBar()
    {
        setUpFileMenu();
        setUpEditMenu();
        setUpAboutMenu();
    }

    // Set up menu items & such

    private void setUpFileMenu()
    {
        fileMenu = new JMenu("File");

        // File menu items
        JMenuItem newMenuItem = new JMenuItem("New");
        newMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem openMenuItem = new JMenuItem("Open");
        openMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem saveMenuItem = new JMenuItem("Save");
        saveMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem saveAsMenuItem = new JMenuItem("Save As");
        saveAsMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem printMenuItem = new JMenuItem("Print");
        printMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        add(fileMenu);
    }

    private void setUpEditMenu() // Create Edit Menu
    {
        editMenu = new JMenu("Edit");
    }

    private void setUpAboutMenu() // Create About Menu
    {
        aboutMenu = new JMenu("About Paint 2.0");
    }
}

I am aware of some of the junk that is in this class that I still need to clean up, but just try and ignore it.

\$\endgroup\$
  • 2
    \$\begingroup\$ You should consider using Swing components, rather than extending Swing components. The only time you should extend a Swing component is when you want to override a method. Your menu bar class looks fine, as long as all the actionPerformed methods are short. If any of your actionPerformed methods gets long, say longer than 10 or 15 lines, you should consider creating a separate ActionListener class. \$\endgroup\$ – Gilbert Le Blanc Aug 28 '13 at 14:10
  • 1
    \$\begingroup\$ Totally agree with Gilbert's comment \$\endgroup\$ – Yuriy Nakonechnyy Aug 28 '13 at 17:38
  • \$\begingroup\$ @GilbertLeBlanc Thanks for the response. If I put up the full code on GitHub or some kind of source-control, would you mind taking a look at it? Your comment has me worried that I've done a few things really wrong. \$\endgroup\$ – hasherr Aug 28 '13 at 22:06
  • \$\begingroup\$ @GilbertLeBlanc: +1. Could you write it as an aswer? \$\endgroup\$ – palacsint Feb 22 '14 at 14:06
2
\$\begingroup\$

+1 to @Gilbert Le Blanc's comment. Creating

I'd consider extracting out methods like createOpenMenuItem(), createSaveMenuItem(), createPrintMenuItem() for better readability and higher abstraction:

private JMenuItem createOpenMenuItem() {
    JMenuItem openMenuItem = new JMenuItem("Open");
    openMenuItem.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {

        }
    });
    return openMenuItem;
}

Then the setUpFileMenu would be a lot cleaner:

private void setUpFileMenu() {
    fileMenu = new JMenu("File");

    // File menu items
    fileMenu.add(createNewMenuItem());
    fileMenu.add(createOpenMenuItem());
    fileMenu.add(createSaveMenuItem());
    fileMenu.add(createSaveAsMenuItem());
    fileMenu.add(createPrintMenuItem());

    add(fileMenu);
}

A maintainer would be helpful, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. For example, finding code of save menu is much faster than before.

Now, each JMenuItem obviously needs an ActionListener, so I just gave each one a different ActionListener. I'm skeptical about this because I know it can be kind of disorganized looking.

I've found this practice easier to work with than having one ActionListener for multiple components (with a switch-case inside).

Basically, I just want somebody to let me know if this practice is all right, or if there is a better alternative that doesn't involve me making 20 new classes.

Having a lot of classes doesn't hurt if they have clear responsibilties.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.