Java Swing Layout Problems & Code Optimisation

Question

In order to train Java I wrote my own program. I like to play CSGO and we never can decide what map to play. So I want to write a program where you can select the maps you want and then it gives out each map from the selection randomly and only once.

There's a bit more to it, but the code for that is working already.

I tried this once already with the Swing Form builder from IntelliJ (my IDE), but as I didn't really understand what it did there, I wanted to redo it by hand. So I rewrote the program, but I can't get it to look properly.

My first Try with IntelliJ Swing Builder:

Without the help of the GUIBuilder it looks like this right now:
So I Googled hours to get the right Layout. Currently I'm using GridBagLayout (seems to fit my needs the most). But I can't get it right. It should look like the first picture.

So, Question 1:

What am I doing wrong with GridBag as there is a lot of unused space and stuff

and Question 2: I know there's a lot to improve codewise, what are your tips? I still have problems knowing when to private and access methods & stuff, so are there any mistakes that are crucial?

Here is my code:

Its 3 Classes:

First
First Main Class (just for starting)

import javax.swing.*;

public class App {

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                JFrame frame = new MainFrame("Map Chooser v0.1");
                frame.setSize(500, 250);
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setVisible(true);
            }
        });
    }
}

** The Swing MainFrame: **

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.ItemEvent;
import java.awt.event.ItemListener;

public class MainFrame extends JFrame {

    ImageIcon dust2;

    public MainFrame(String title) {
        super(title);

        GridLayout gridMain = new GridLayout(10, 10, 5, 5);
        GridLayout gridCB = new GridLayout(3, 5, 5, 5);
        GridLayout gridButton = new GridLayout(1, 1, 10, 10);

        JPanel mainPanel = new JPanel();
        mainPanel.setLayout(gridMain);
        mainPanel.setBackground(Color.DARK_GRAY);

        JPanel checkboxPanel = new JPanel();
        checkboxPanel.setLayout(gridCB);
        checkboxPanel.setBackground(Color.DARK_GRAY);

        JPanel buttonPanel = new JPanel();
        buttonPanel.setLayout(gridButton);
        buttonPanel.setBackground(Color.DARK_GRAY);

        JPanel labelPanel = new JPanel();
        labelPanel.setBackground(Color.DARK_GRAY);

        mainPanel.add(checkboxPanel);
        mainPanel.add(buttonPanel);
        mainPanel.add(labelPanel);

        MapRound map = new MapRound();
        //GridBagConstraints gbc = new GridBagConstraints();
        GridBagConstraints gbc2 = new GridBagConstraints();
        GridBagConstraints gbc3 = new GridBagConstraints();
        gbc2.weighty = 0.5;
        gbc2.weightx = 0.5;

        gbc3.weightx = 1;
        gbc3.weighty = 1;
        gbc3.fill = GridBagConstraints.ABOVE_BASELINE;

        //Checkboxes
        JCheckBox dust2CB = new JCheckBox("Dust II");
        dust2CB.setBackground(Color.DARK_GRAY);
        dust2CB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 0;
        gbc2.gridy = 0;
        checkboxPanel.add(dust2CB, gbc2);
        dust2CB.setSelected(false);

        JCheckBox trainCB = new JCheckBox("Train");
        trainCB.setBackground(Color.DARK_GRAY);
        trainCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 0;
        gbc2.gridy = 1;
        checkboxPanel.add(trainCB, gbc2);
        trainCB.setSelected(false);

        JCheckBox mirageCB = new JCheckBox("Mirage");
        mirageCB.setBackground(Color.DARK_GRAY);
        mirageCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 0;
        gbc2.gridy = 2;
        checkboxPanel.add(mirageCB, gbc2);
        mirageCB.setSelected(false);

        JCheckBox infernoCB = new JCheckBox("Inferno");
        infernoCB.setBackground(Color.DARK_GRAY);
        infernoCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 1;
        gbc2.gridy = 0;
        checkboxPanel.add(infernoCB, gbc2);
        infernoCB.setSelected(false);

        JCheckBox cobblestoneCB = new JCheckBox("Cobblestone");
        cobblestoneCB.setBackground(Color.DARK_GRAY);
        cobblestoneCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 1;
        gbc2.gridy = 1;
        checkboxPanel.add(cobblestoneCB, gbc2);
        cobblestoneCB.setSelected(false);

        JCheckBox overpassCB = new JCheckBox("Overpass");
        overpassCB.setBackground(Color.DARK_GRAY);
        overpassCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 1;
        gbc2.gridy = 2;
        checkboxPanel.add(overpassCB, gbc2);
        overpassCB.setSelected(false);

        JCheckBox cacheCB = new JCheckBox("Cache");
        cacheCB.setBackground(Color.DARK_GRAY);
        cacheCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 2;
        gbc2.gridy = 0;
        checkboxPanel.add(cacheCB, gbc2);
        cacheCB.setSelected(false);

        JCheckBox aztecCB = new JCheckBox("Aztec");
        aztecCB.setBackground(Color.DARK_GRAY);
        aztecCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 2;
        gbc2.gridy = 1;
        checkboxPanel.add(aztecCB, gbc2);
        aztecCB.setSelected(false);

        JCheckBox dustCB = new JCheckBox("Dust");
        dustCB.setBackground(Color.DARK_GRAY);
        dustCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 2;
        gbc2.gridy = 2;
        checkboxPanel.add(dustCB, gbc2);
        dustCB.setSelected(false);

        JCheckBox vertigoCB = new JCheckBox("Vertigo");
        vertigoCB.setBackground(Color.DARK_GRAY);
        vertigoCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 3;
        gbc2.gridy = 0;
        checkboxPanel.add(vertigoCB, gbc2);
        vertigoCB.setSelected(false);

        JCheckBox nukeCB = new JCheckBox("Nuke");
        nukeCB.setBackground(Color.DARK_GRAY);
        nukeCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 3;
        gbc2.gridy = 1;
        checkboxPanel.add(nukeCB, gbc2);
        nukeCB.setSelected(false);

        JCheckBox officeCB = new JCheckBox("Office");
        officeCB.setBackground(Color.DARK_GRAY);
        officeCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 3;
        gbc2.gridy = 2;
        checkboxPanel.add(officeCB, gbc2);
        officeCB.setSelected(false);

        JCheckBox italyCB = new JCheckBox("Italy");
        italyCB.setBackground(Color.DARK_GRAY);
        italyCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 9;
        gbc2.gridy = 0;
        checkboxPanel.add(italyCB, gbc2);
        italyCB.setSelected(false);

        JCheckBox assaultCB = new JCheckBox("Assault");
        assaultCB.setBackground(Color.DARK_GRAY);
        assaultCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 9;
        gbc2.gridy = 1;
        checkboxPanel.add(assaultCB, gbc2);
        assaultCB.setSelected(false);

        JCheckBox militiaCB = new JCheckBox("Militia");
        militiaCB.setBackground(Color.DARK_GRAY);
        militiaCB.setForeground(Color.LIGHT_GRAY);
        gbc2.gridx = 9;
        gbc2.gridy = 2;
        checkboxPanel.add(militiaCB, gbc2);
        militiaCB.setSelected(false);

        //Buttons
        JButton startButton = new JButton("Start");
        startButton.setBackground(Color.DARK_GRAY);
        startButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 0;
        gbc3.insets = new Insets(10, 10, 10, 10);
        startButton.setPreferredSize(new Dimension(400, 400));
        buttonPanel.add(startButton, gbc3);

        JButton newmapButton = new JButton("New Map");
        newmapButton.setBackground(Color.DARK_GRAY);
        newmapButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 1;
        gbc3.weightx = 1.0;
        gbc3.weighty = 1.0;
        gbc3.insets = new Insets(0, 0, 10, 10);
        buttonPanel.add(newmapButton, gbc3);
        newmapButton.setEnabled(false);

        JButton resetButton = new JButton("Reset");
        resetButton.setBackground(Color.DARK_GRAY);
        resetButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 3;
        gbc3.insets = new Insets(0, 0, 10, 10);
        buttonPanel.add(resetButton, gbc3);
        resetButton.setEnabled(false);

        //Labels
        JLabel result = new JLabel("Press START to begin");
        gbc3.gridx = 0;
        gbc3.gridy = 4;
        gbc3.insets = new Insets(0, 0, 10, 10);
        labelPanel.add(result, gbc3);

        //Verhalten
        dust2CB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {

                if (dust2CB.isSelected()) {
                    map.mList.add("DustII");
                } else {
                    map.removeMap("DustII");
                }
            }
        });

        trainCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (trainCB.isSelected()) {
                    map.mList.add("Train");
                } else {
                    map.removeMap("Train");
                }
            }
        });

        mirageCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (mirageCB.isSelected()) {
                    map.mList.add("Mirage");
                } else {
                    map.removeMap("Mirage");
                }
            }
        });

        infernoCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (infernoCB.isSelected()) {
                    map.mList.add("Inferno");
                } else {
                    map.removeMap("Inferno");
                }
            }
        });

        cobblestoneCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (cobblestoneCB.isSelected()) {
                    map.mList.add("Cobblestone");
                } else {
                    map.removeMap("Cobblestone");
                }
            }
        });

        overpassCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (overpassCB.isSelected()) {
                    map.mList.add("Overpass");
                } else {
                    map.removeMap("Overpass");
                }
            }
        });

        cacheCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (cacheCB.isSelected()) {
                    map.mList.add("Cache");
                } else {
                    map.removeMap("Cache");
                }
            }
        });

        aztecCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (aztecCB.isSelected()) {
                    map.mList.add("Aztec");
                } else {
                    map.removeMap("Aztec");
                }
            }
        });

        dustCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (dustCB.isSelected()) {
                    map.mList.add("Dust");
                } else {
                    map.removeMap("Dust");
                }
            }
        });

        vertigoCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (vertigoCB.isSelected()) {
                    map.mList.add("Vertigo");
                } else {
                    map.removeMap("Vertigo");
                }
            }
        });

        nukeCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (nukeCB.isSelected()) {
                    map.mList.add("Nuke");
                } else {
                    map.removeMap("Nuke");
                }
            }
        });

        officeCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (officeCB.isSelected()) {
                    map.mList.add("Office");
                } else {
                    map.removeMap("Office");
                }
            }
        });

        italyCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (italyCB.isSelected()) {
                    map.mList.add("Italy");
                } else {
                    map.removeMap("Italy");
                }
            }
        });

        assaultCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (assaultCB.isSelected()) {
                    map.mList.add("Assault");
                } else {
                    map.removeMap("Assault");
                }
            }
        });

        militiaCB.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (militiaCB.isSelected()) {
                    map.mList.add("Militia");
                } else {
                    map.removeMap("Militia");
                }
            }
        });

        startButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e2) {

                if (map.mList.size() == 0) {
                    JOptionPane.showMessageDialog
                            (null, "Please add one or more    maps to the selection pool!");
                } else {

                    map.scambleMap();
                    startButton.setEnabled(false);
                    newmapButton.setEnabled(true);
                    resetButton.setEnabled(true);

                    dust2CB.setEnabled(false);
                    trainCB.setEnabled(false);
                    mirageCB.setEnabled(false);
                    infernoCB.setEnabled(false);
                    cobblestoneCB.setEnabled(false);
                    overpassCB.setEnabled(false);
                    cacheCB.setEnabled(false);
                    aztecCB.setEnabled(false);
                    dustCB.setEnabled(false);
                    vertigoCB.setEnabled(false);
                    nukeCB.setEnabled(false);
                    officeCB.setEnabled(false);
                    italyCB.setEnabled(false);
                    assaultCB.setEnabled(false);
                    militiaCB.setEnabled(false);
                }
            }
        });

        newmapButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e2) {
                boolean preventloop = true;

                try {
                    while (preventloop) {
                        String xyz = map.nextMap();
                        result.setText("<html><font size=10><font color=green>
                                "+ xyz + </font></font></html>");
                        preventloop = false;
                    }
                } catch (Exception e) {
                    JOptionPane.showMessageDialog(null,
                            "You played all maps!\nThe game has been reset");
                    map.resetStatus();
                    result.setText("Press START to begin");
                    newmapButton.setEnabled(false);
                    startButton.setEnabled(true);
                    dust2CB.setEnabled(true);
                    trainCB.setEnabled(true);
                    mirageCB.setEnabled(true);
                    infernoCB.setEnabled(true);
                    cobblestoneCB.setEnabled(true);
                    overpassCB.setEnabled(true);
                    cacheCB.setEnabled(true);
                    aztecCB.setEnabled(true);
                    dustCB.setEnabled(true);
                    vertigoCB.setEnabled(true);
                    nukeCB.setEnabled(true);
                    officeCB.setEnabled(true);
                    italyCB.setEnabled(true);
                    assaultCB.setEnabled(true);
                    militiaCB.setEnabled(true);
                }
            }
        });

        resetButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {

                map.resetStatus();
                result.setText("Press START to begin");
                newmapButton.setEnabled(false);
                startButton.setEnabled(true);

                dust2CB.setEnabled(true);
                trainCB.setEnabled(true);
                mirageCB.setEnabled(true);
                infernoCB.setEnabled(true);
                cobblestoneCB.setEnabled(true);
                overpassCB.setEnabled(true);
                cacheCB.setEnabled(true);
                aztecCB.setEnabled(true);
                dustCB.setEnabled(true);
                vertigoCB.setEnabled(true);
                nukeCB.setEnabled(true);
                officeCB.setEnabled(true);
                italyCB.setEnabled(true);
                assaultCB.setEnabled(true);
                militiaCB.setEnabled(true);
            }
        });

        this.add(mainPanel);
        this.setVisible(true);
    }
}

And the last: (Blueprint for Map Object which containes the Arraylist

import java.util.ArrayList; import java.util.Collections;

public class MapRound { ArrayList mList = new ArrayList<>(); int counter = 0;

//Constructor
public MapRound() {
}

public String nextMap() {
    String result = mList.get(counter);
    counter = counter + 1;
    return result;
}

public void resetStatus() {
    counter = 0;
    Collections.shuffle(mList);
}

public String removeMap(String index) {
    mList.remove(index);
    return index;
}

public void scambleMap() {
    Collections.shuffle(mList);
}

}

If you have any ideas or advices, I would be happy to hear them!

Answer 1

Looking at your code, I see the GridLayout layout manager being used (instead of GridBagLayout). Although the names are very similar, their behavior is very different. This also means that the GridBagConstraints objects you pass along when adding a component to one of the panels will not be understood properly. (It would be nice if the layout manager would give an error message when a constraints object is passed that it cannot handle.)

See for example this nice Visual Guide to Layout Managers for more information on these two (and other) layout managers.

I think the BoxLayout layout manager and empty borders around some panels would be useful for your program too. You can use labels to show the images at the top and bottom of your GUI. I think that all GridBagConstraints related code can be removed. An example screenshot:

Your MainFrame class could be modified like this:

public class MainFrame extends JFrame {

    //ImageIcon dust2;
    public MainFrame(String title) {
        super(title);

        //GridLayout gridMain = new GridLayout(5, 0, 5, 5);
        GridLayout gridCB = new GridLayout(3, 5, 5, 5);
        //GridLayout gridButton = new GridLayout(4, 1, 10, 10);

        JPanel mainPanel = new JPanel();
        BoxLayout mainLayout = new BoxLayout(mainPanel, BoxLayout.PAGE_AXIS);
        mainPanel.setLayout(mainLayout);
        mainPanel.setBackground(Color.DARK_GRAY);

        JPanel checkboxPanel = new JPanel();
        checkboxPanel.setLayout(gridCB);
        checkboxPanel.setBackground(Color.DARK_GRAY);

        JPanel buttonPanel = new JPanel();
        BoxLayout buttonLayout = new BoxLayout(buttonPanel, BoxLayout.PAGE_AXIS);
        buttonPanel.setLayout(buttonLayout);
        buttonPanel.setBorder(new EmptyBorder(6, 6, 6, 6));
        buttonPanel.setBackground(Color.DARK_GRAY);

        JPanel labelPanel = new JPanel();
        labelPanel.setBackground(Color.DARK_GRAY);

        String directory = "directory/to/images/";

        JPanel topImagePanel = new JPanel();
        topImagePanel.setBackground(Color.DARK_GRAY);
        topImagePanel.setBorder(new EmptyBorder(6, 6, 6, 6));
        String topImagePath = directory + "map selector - top.png";
        topImagePanel.add(new JLabel(new ImageIcon(topImagePath)));

        JPanel bottomImagePanel = new JPanel();
        bottomImagePanel.setBackground(Color.DARK_GRAY);
        bottomImagePanel.setBorder(new EmptyBorder(6, 6, 6, 6));
        String bottomImagePath = directory + "map selector - bottom.png";
        bottomImagePanel.add(new JLabel(new ImageIcon(bottomImagePath)));

        mainPanel.add(topImagePanel);
        mainPanel.add(checkboxPanel);
        mainPanel.add(buttonPanel);
        mainPanel.add(labelPanel);
        mainPanel.add(bottomImagePanel);

        // [...]

        //Buttons
        JButton startButton = new JButton("Start");
        startButton.setBackground(Color.DARK_GRAY);
        startButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 0;
        gbc3.insets = new Insets(10, 10, 10, 10);
        startButton.setPreferredSize(new Dimension(400, 400));
        buttonPanel.add(startButton, gbc3);
        startButton.setAlignmentX(Component.CENTER_ALIGNMENT);

        buttonPanel.add(Box.createRigidArea(new Dimension(0, 6)));

        JButton newmapButton = new JButton("New Map");
        newmapButton.setBackground(Color.DARK_GRAY);
        newmapButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 1;
        gbc3.weightx = 1.0;
        gbc3.weighty = 1.0;
        gbc3.insets = new Insets(0,0,10,10);
        buttonPanel.add(newmapButton, gbc3);
        newmapButton.setAlignmentX(Component.CENTER_ALIGNMENT);
        newmapButton.setEnabled(false);

        buttonPanel.add(Box.createRigidArea(new Dimension(0, 6)));

        JButton resetButton = new JButton("Reset");
        resetButton.setBackground(Color.DARK_GRAY);
        resetButton.setForeground(Color.LIGHT_GRAY);
        gbc3.gridx = 0;
        gbc3.gridy = 3;
        gbc3.insets = new Insets(0,0,10,10);
        buttonPanel.add(resetButton, gbc3);
        resetButton.setAlignmentX(Component.CENTER_ALIGNMENT);
        resetButton.setEnabled(false);

        // [...]
    }
}

Edit: some general advice for your code

• The MainFrame constructor is very large. I would split it up in smaller methods to create the different panels and to add the listeners.
• Search for similar pieces of code to see if you can make a reusable method. For example: there are three blocks of code that set all check boxes to disabled or enabled. You could make a method with a boolean parameter to call setEnabled for all check boxes.
• Creating the check boxes currently takes a lot of code. If you have a list of game maps, you could create the check boxes in a loop. (This is a nice advantage of creating your GUI in code.) If you use the name property of each check box, they could all share the same ItemListener. And finally, if you store all the check boxes in a list, you can easily disable or enable all of them.

Some code to illustrate this:

java.util.List<String> gameMaps = Arrays.asList(
        "Dust II", "Train", "Mirage", "Inferno", "Cobblestone", "Overpass",
        "Cache", "Aztec", "Dust", "Vertigo", "Nuke", "Office", "Italy",
        "Assault", "Militia"
);

ItemListener mapCheckBoxListener = new ItemListener() {
    @Override
    public void itemStateChanged(ItemEvent itemEvent) {
        JCheckBox checkBox = (JCheckBox) itemEvent.getSource();
        if (checkBox.isSelected()) {
            map.mList.add(checkBox.getName());
        } else {
            map.removeMap(checkBox.getName());
        }
    }
};

java.util.List<JCheckBox> mapCheckBoxes = new ArrayList<>();
for (String gameMap : gameMaps) {
    JCheckBox mapCheckBox = new JCheckBox(gameMap);
    mapCheckBox.setName(gameMap);
    mapCheckBox.setBackground(Color.DARK_GRAY);
    mapCheckBox.setForeground(Color.LIGHT_GRAY);
    checkboxPanel.add(mapCheckBox);
    mapCheckBox.setSelected(false);
    mapCheckBox.addItemListener(mapCheckBoxListener);
    mapCheckBoxes.add(mapCheckBox);
}

// [....]

private void setMapsEnabled(java.util.List<JCheckBox> mapCheckBoxes, boolean enabled) {
    for (JCheckBox mapCheckBox : mapCheckBoxes) {
        mapCheckBox.setEnabled(enabled);
    }
}