Swing UI for database-to-Excel tool

Question

I'm just wondering what is the cleanest way you can write your GUI/Swing code? Right below is the code I have for my main frame. Could you guys let me know if it looks clean/understandable, and if there is any way I can improve it?

The code I have provided is the main frame of my program, which is basically a GUI program that allows users to open different databases, select tables, and then the columns they would like to inner-join. I have a DatabaseView, and a TableView as well. This is my main view. I'm just wondering if I'm doing it properly (organization-wise). Like, if the ActionListeners are where they're supposed to be, how I grouped the customizers, and Actions, etc.

"Export" means exporting the selected tables' columns (inner-joined) to Excel.

public class MainView {
    private JFrame frame;
    private JPanel dbPanel;
    private JPanel previewPanel;
    private MainController controller;
    private JSplitPane splitPane;
    private JTextField txtCondition;
    private JComboBox<String> dbChoices;
    private CardLayout cardLayout;

    public MainView() {
        try {
            frame = new JFrame();
            initFrame();
            frame.setVisible(true);
            UIManager.setLookAndFeel("com.sun.java.swing.plaf.nimbus.NimbusLookAndFeel");
        }
        catch (ClassNotFoundException | InstantiationException | IllegalAccessException
                | UnsupportedLookAndFeelException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    private void initFrame() {
        customizeFrame();

        cardLayout = new CardLayout();
        dbPanel = new JPanel(cardLayout);
        previewPanel = new JPanel();

        splitPane = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT, dbPanel, previewPanel);
        customizeSplitPane(splitPane);

        dbChoices = new JComboBox<String>();
        txtCondition = new JTextField();

        JMenuBar menuBar = new JMenuBar();

        JMenu mnFile = new JMenu("File");
        JMenuItem mntmConnectToDB = new JMenuItem("Open a Database");
        JMenuItem mntmDisconnectDB = new JMenuItem("Disconnect Database");
        JSeparator separator = new JSeparator();
        JMenuItem mntmExit = new JMenuItem("Exit");

        JMenu mnOptions = new JMenu("Options");
        JMenuItem mntmExport = new JMenuItem("Export");

        JMenu mnHelp = new JMenu("Help");
        JMenuItem mntmAbout = new JMenuItem("About");

        JToolBar toolBar = new JToolBar();
        JButton btnOpenDatabase = new JButton("Open Database");
        JButton btnCloseDatabase = new JButton("Close Database");
        JButton btnExport = new JButton("Export");

        Label lblSelectDB = new Label("Select Database:");
        JLabel lblEnterCondition = new JLabel("Enter condition: WHERE ");

        /*---------------------- Default settings ----------------------*/

        mntmDisconnectDB.setEnabled(false);
        dbChoices.setBounds(173, 29, 184, 20);
        toolBar.setBounds(0, 0, 874, 23);
        lblSelectDB.setBounds(10, 29, 146, 22);
        lblEnterCondition.setBounds(10, 452, 146, 20);
        txtCondition.setBounds(167, 452, 190, 20);
        txtCondition.setColumns(10);

        /*---------------------- Action Listeners ----------------------*/

        mntmConnectToDB.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                connectBtnActionPerformed(evt);
            }
        });

        btnOpenDatabase.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                connectBtnActionPerformed(evt);
            }
        });

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

        mntmExit.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                closeBtnActionPerformed(evt);
            }
        });

        dbChoices.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                selectDBActionPerformed(evt);
            }
        });

        mntmAbout.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent evt) {
                aboutBtnActionPerformed(evt);
            }
        });

        /*--------------------------------------------------------------*/
        frame.setJMenuBar(menuBar);

        menuBar.add(mnFile);
        mnFile.add(mntmConnectToDB);
        mnFile.add(mntmDisconnectDB);
        mnFile.add(separator);
        mnFile.add(mntmExit);

        menuBar.add(mnOptions);
        mnOptions.add(mntmExport);

        menuBar.add(mnHelp);
        mnHelp.add(mntmAbout);

        frame.getContentPane().add(splitPane);

        frame.getContentPane().add(toolBar);        
        toolBar.add(btnOpenDatabase);
        toolBar.add(btnCloseDatabase);
        toolBar.add(btnExport);

        frame.getContentPane().add(lblSelectDB);
        frame.getContentPane().add(dbChoices);

        frame.getContentPane().add(txtCondition);
        frame.getContentPane().add(lblEnterCondition);      
    }

    public void addController(MainController c) {
        controller = c;
    }

    public void addDBchoice(String dbName, DatabaseView dbView) {
        dbChoices.addItem(dbName);
        dbPanel.add(dbView, dbName);
    }

    /*---------------------- View Customizers ----------------------*/

    private void customizeFrame() {
        frame.setTitle("DB to Excel");
        frame.setSize(890, 541);
        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().setLayout(null);
    }

    private JSplitPane customizeSplitPane(JSplitPane splitPane) {
        splitPane.setBounds(0, 62, 874, 379);
        splitPane.setOneTouchExpandable(true);
        splitPane.setDividerLocation(600);
        return splitPane;
    }

    /*-------------------------- Actions --------------------------*/

    private void closeBtnActionPerformed(ActionEvent evt) {
        System.exit(0);
    }

    private void connectBtnActionPerformed(ActionEvent evt) {
        controller.connectDatabase();
    }

    private void selectDBActionPerformed(ActionEvent evt) {
        String selectedDB = (String) ((JComboBox<String>) evt.getSource()).getSelectedItem();
        System.out.println("Selected DB: " + selectedDB);
        cardLayout.show(dbPanel, selectedDB);
    }

    private void aboutBtnActionPerformed(ActionEvent evt) {
        controller.showAbout();
    }

}

Answer 1

Minimize failure scopes

Try-Blocks should be as small as reasonably possible. Some people even advocate extracting error-handling into a separate method of the form:

void doSomething() {
    try {
        something();
    } catch (RelevantException | AnotherRelevantException e) {
        // handle appropriately
    }
}

as such the try-block in your constructor should be:

    public MainView() {
        frame = new JFrame();
        initFrame();
        frame.setVisible(true);
        try {
          UIManager.setLookAndFeel("com.sun.java.swing.plaf.nimbus.NimbusLookAndFeel");
        }
        catch (ClassNotFoundException | InstantiationException | IllegalAccessException
                | UnsupportedLookAndFeelException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

Constructors: dos and don'ts

Let's recapitulate the responsibilities of a constructor:

• Object initialization into a usable state
• ...

right. nothing else. A constructor is only responsible for getting the Object into a usable state. This means, no frame.setVisible(true); in the constructor.

Also it's often easier and clearer to eagerly initialize members of a class. This allows your constructor body to be laser focused on what you actually want to do.

so frame = new JFrame(); vanishes, too.

Also not in the responsibilities of a constructor: Setting global state. Modifications to the UIManager belong into your main-method. Don't abuse the constructor for that.

With all these changes your constructor shrinks considerably to:

public MainView() {
    initFrame();
}

Now what we could (and probably should do) is stop hiding the layouting setup in a separate method doing everything, but inline this monster (Eclipse has Alt+Shift+I) and then look at what we're doing there.

Layout initializations

OOP-Expert has written a nice section about that in their answer, I'd like to point out an alternative though.

While the separation of "components we need again" and "components we need to setup once" is important and useful, I'd like to stay away from lazy initialization.

You have these blocks of logically coherent steps in initFrame, you even marked them with comments.

These blocks are probably well off as extracted methods. I personally like to create the necessary component for usage in "factory-methods", if they don't need to be members of my class. For all others, I usually use some separate method to run a layout over them.

Again eager initialization is the key to not being suprised by things happening. You should also consider making as many members as possible final to not accidentally change them in your code.

For a more in-depth review of the layout initialization, check my other answer

Sidenote: This is all personal preference in here, YMMV

Use Lambdas and Method References

The block "Action Listeners" could be significantly shortened, when you use Lambdas, or even method references there. A good IDE will even tell you about opportunities to simplify your code that way.

    mntmConnectToDB.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent evt) {
            connectBtnActionPerformed(evt);
        }
    });

could simply be:

mntmConnectToDB.addActionListener(this::connectBtnActionPerformed);

This reminds me: BtnActionPerformed is basically useless for all these methods. Don't name your methods after what they are, or when they are invoked but rather by what they do. That makes it easier to grasp what happens and usually is also more succinct.

Ending the Application

Stay away from System.exit to close your applications. That's a jerk move and you should only do it if you don't have any other choice.

Instead of shooting the whole JVM, you should enqueue a WINDOW_CLOSING Event, see how in this Stack Overflow post. This basically goes back down to your DefaultCloseOperation and runs that, instead of nuking the JVM (unless you say EXIT_ON_CLOSE, but eh :D)

Answer 2

1. Localize creation/lazy intialization for UI components

This should be done to satisfy the single responsible principle. Things that have high cohesion should be strongly coupled. In this case: the construction and the initial configuration.

Creation method for UI components that do not need to be accessable anymore:

private JButton createBtnOpenDatabase() {
    JButton btnOpenDatabase = new JButton("Open Database");
    btnOpenDatabase.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent evt) {
            connectBtnActionPerformed(evt);
        }
    });
    return btnOpenDatabase;
}

Lazy intitialization of object local variable for UI components that still need to be accessable:

private JTextField txtCondition;

private JTextField getTxtCondition() {
    if (this.txtCondition == null) {
        this.txtCondition = new JTextField();
        this.txtCondition.setBounds(167, 452, 190, 20);
        this.txtCondition.setColumns(10);
    }
    return this.txtCondition;
}

After you use lazy init every access to txtCondition must be done through the getter.

Tie up construction and initial configuration. Refactor

splitPane = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT, dbPanel, previewPanel);
customizeSplitPane(splitPane);

to lazy init structure

private JSplitPane getSplitPane() {
    if (splitPane == null) {
        splitPane = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT, getDbPanel() /*lazy*/, getPreviewPanel() /*lazy*/);
        splitPane.setBounds(0, 62, 874, 379);
        splitPane.setOneTouchExpandable(true);
        splitPane.setDividerLocation(600);
    }
    return splitPane;
}

2. Create components

After you identified strong cohesional code fragments through create-methods and lazy init-methods you should extract logical UI components. The most obvious component is the JFrame. You should derive a class from it and build up the UI within this new XJframe. You already have a class named MainView that can be extended from JFrame. Maybe you can reuse it so the inner JFrame-object becomes obsolete. I recommend to at least do that:

class MainView extends JFrame

Some other components may be the preview panel or the db panel. But think hard about it because the smaller the components the more effort will have to communicate state changes through observer patterns to keep the components independent.

3. Harmonize controller access

Two of your action methods do not ask the controller.

private void closeBtnActionPerformed(ActionEvent evt) {
    System.exit(0);
}

and

private void selectDBActionPerformed(ActionEvent evt) {
    String selectedDB = (String) ((JComboBox<String>) evt.getSource())
            .getSelectedItem();
    System.out.println("Selected DB: " + selectedDB);
    cardLayout.show(dbPanel, selectedDB);
}

Those methods should delegate their requests to the controller and the controller should tell the UI what to do. For example: currently you will confuse a developer where to put the logic when adding a requirement that will avoid closing under special circumstances.

4. Make the controller the responsible to orchestrate

The controller should be responsible for requesting the UI to put itself in a specific state. So the controller should be the constructor parameter of your UI and it should not be set by a setter. The controller lives first, then the UI. This is because it may not be necessary to show a UI through business rules of the business model the controller asked for.

Remove "void addController(...);".

5. Avoid "System.exit(0);"

This statement makes either no sense or it is hiding programming errors like running threads that have been forgotten to shut down.

To make it clear: System.exit(1); can make sense if you want to communicate an error state to the OS.

Use following to let Swing disassemble your UI when the closing request was initiated via X:

frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

For a button to make this job:

private void closeBtnActionPerformed(ActionEvent evt) {
    getFrame()/*lazy*/.dispose();
}

Answer 3

Okay the Layouting earns itself a whole answer after I've seen it in much detail...

Eager initialization:

This is somewhat a continuation of my earlier answer, where I already criticised this.

You can speed this up significantly by actually going over all the members referenced in your initFrame() (which I inlined to the constructor for the reasons given in the other answer), and just making them eagerly initialized.

With a little reordering that even works just fine.

Component cohesion.

It's important for code-understanding to be allowed to "ignore" some things in the code. This is especially helpful when you work with multiple variables as in your frame setup.

Instead of:

JMenu mnFile = new JMenu("File");
JMenuItem mntmConnectToDB = new JMenuItem("Open a Database");
JMenuItem mntmDisconnectDB = new JMenuItem("Disconnect Database");
// skipping ~ 20 LoC

/*---------------------- Default settings ----------------------*/
mntmDisconnectDB.setEnabled(false);
dbChoices.setBounds(173, 29, 184, 20);
// and another 5 lines

mntmConnectToDB.addActionListener(new ActionListener() {
   public void actionPerformed(ActionEvent evt) {
        connectBtnActionPerformed(evt);
    }
});
// another 20 or even more and then finally 
menuBar.add(mnFile);
mnFile.add(mntmConnectToDB);
mnFile.add(mntmDisconnectDB);

wow... that's one UI component stretched across some 70 Lines of code and intertwined with at least 10 other components. Nobody can grasp that in their heads... nobody

pack your components together, finish one component, then do the next. This makes your code form cohesive blocks that refer to the same component, something like:

    JToolBar toolBar = new JToolBar();
    toolBar.setBounds(0, 0, 874, 23);
    JButton btnOpenDatabase = new JButton("Open Database");
    btnOpenDatabase.addActionListener(this::connectBtnActionPerformed);

    JButton btnCloseDatabase = new JButton("Close Database");
    btnCloseDatabase.addActionListener(e -> {});

    JButton btnExport = new JButton("Export");

    toolBar.add(btnOpenDatabase);
    toolBar.add(btnCloseDatabase);
    toolBar.add(btnExport);

    frame.getContentPane().add(toolBar);

and at this point it should jump in your face: Extract that into a method:

frame.add(buildToolbar());

// .. somewhere else:
private JToolBar buildToolbar() {
    // all that code above and then just
    return toolBar;
}

Minor gimmicks:

• As mentioned in the JFrame Javadocs, jframe.add() is a shorthand for jframe.getContentPane().add(). You should use it.

• There is no need to bind a no-op ActionListener... remove btnCloseDatabase.addActionListener(...

• Hardcoding numbers is generally ... unflexible. You should strive to make your UI adapt to changes in it's size. Magic numbers all over your code don't help. Try to get rid of them

• txtCondition is not used anywhere besides the layout initialization. It doesn't need to be a field.

• Setting the LayoutManager to null on something should not be necessary. Get rid of that line.
  Similarly applies to locationRelativeTo...

End result

After these changes and some others outlined in the previously mentioned answer, the first few lines of your class currently look like this:

public class MainView {
    private JFrame frame = new JFrame();
    private CardLayout cardLayout = new CardLayout();
    private JPanel dbPanel = new JPanel(cardLayout);
    private JPanel previewPanel = new JPanel();
    private MainController controller;
    private JSplitPane splitPane = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT, dbPanel, previewPanel);
    private JComboBox<String> dbChoices = new JComboBox<>();

    public MainView() {
        frame.setTitle("DB to Excel");
        frame.setSize(890, 541);
        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().setLayout(null);
        frame.setJMenuBar(buildMenuBar());

        splitPane.setBounds(0, 62, 874, 379);
        splitPane.setOneTouchExpandable(true);
        splitPane.setDividerLocation(600);
        dbChoices.setBounds(173, 29, 184, 20);
        dbChoices.addActionListener(this::selectDBActionPerformed);

        Label lblSelectDB = new Label("Select Database:");
        lblSelectDB.setBounds(10, 29, 146, 22);

        JLabel lblEnterCondition = new JLabel("Enter condition: WHERE ");
        lblEnterCondition.setBounds(10, 452, 146, 20);

        JTextField txtCondition = new JTextField();
        txtCondition.setBounds(167, 452, 190, 20);
        txtCondition.setColumns(10);

        frame.add(splitPane);
        frame.add(buildToolbar());

        frame.add(lblSelectDB);
        frame.add(dbChoices);

        frame.add(txtCondition);
        frame.add(lblEnterCondition);
    }
// class continues here