What is wrong with my loops?

Question

So I'm trying to "optimize" my code, by reducing the number of lines, by using lots of loops. But it's been.....problematic.

I'm just trying to display a bunch of radio buttons, only two buttons appear and neither do anything.

Just wondered if someone could critique my code and show this newb where he is going wrong.

Grateful for any help :)

import java.awt.event.*;
import javax.swing.*;  
import javax.swing.JOptionPane;
import javax.swing.JFrame;
import javax.swing.JLabel;
import java.awt.BorderLayout;
import java.awt.Color;


public class NewGame implements ActionListener {

    private final String[] premierLeagueClubs = {"Arsenal", "Bournemouth", "Burnley", "Chelsea", "Crystal Palace",
        "Everton", "Hull City", "Leicester City", "Liverpool", "Manchester United", "Manchester City", "Middlesborough",
        "Southampton", "Stoke City", "Sunderland", "Swansea City", "Tottenham Hotspur", "Watford", "West Brom", "West Ham"};
    private final JRadioButton[] rb = new JRadioButton[20];
    JFrame f3;
    private final JButton b, quit;
    String teamName;
    JLabel label1;
    private static JPanel chooseClubPanel;

    public NewGame() {
        f3 = new JFrame("Ballon d'or");
        f3.setExtendedState(JFrame.MAXIMIZED_BOTH);
        f3.setDefaultCloseOperation(WindowConstants.DO_NOTHING_ON_CLOSE);

        chooseClubPanel = new JPanel();
        f3.add(chooseClubPanel, BorderLayout.CENTER);
        chooseClubPanel.setBackground(Color.GREEN);

        label1 = new JLabel("Please choose a team");
        chooseClubPanel.add(label1);

        ButtonGroup bg = new ButtonGroup();

        int startvalueBG;
        int endvalueBG = 19;
        for (startvalueBG = 0; startvalueBG <= endvalueBG; startvalueBG++) {
            bg.add(rb[startvalueBG]);
        }

        b = new JButton("OK");
        b.addActionListener(this);
        chooseClubPanel.add(b);

        quit = new JButton("Quit");
        quit.addActionListener(this);
        chooseClubPanel.add(quit);

        int startValueAddRB;
        int endValueAddRB = 19;
        for (startValueAddRB = 0; startValueAddRB <= endValueAddRB; startValueAddRB++) {
            f3.add(rb[startValueAddRB]);
        }

        int startRB;
        int endRB = 19;

        for (startRB = 0; endRB <= 19; startRB++) {
            for (int i = 0; i < premierLeagueClubs.length; i++) {
                rb[startRB] = new JRadioButton(premierLeagueClubs[i]);
                chooseClubPanel.add(rb[i]);
            }
        }
        f3.addWindowListener(new WindowAdapter() {

            @Override
            public void windowClosing(WindowEvent e) {
                if (JOptionPane.showConfirmDialog(f3, "Are you sure ?", "Warning", JOptionPane.YES_NO_OPTION) == JOptionPane.YES_OPTION) {
                    f3.setVisible(false);
                    f3.dispose();
                } else {

                    f3.setVisible(true);
                }
            }
        });
    }

    public void setFrame(JFrame f3) {
        this.f3 = f3;
    }

    public JFrame getFrame() {

        return f3;
    }

    @Override
    public void actionPerformed(ActionEvent e) {

        int rbNumber;
        int rbNumberMax = 19;

        for (rbNumber = 0; rbNumber < rbNumberMax; rbNumber++) {
            if (rb[rbNumber].isSelected()) {
                teamName = rb[rbNumber].getText();
                f3.dispose();
                JOptionPane.showMessageDialog(f3, "You chose : " + rb[rbNumber].getText());

            }
        }

    }

}

Answer 1

So, let's take a look at the code...

First, you try and add each button to the ButtonGroup, but, you've not actually created any yet, possible NullPointerException

int startvalueBG;
int endvalueBG = 19;
for (startvalueBG = 0; startvalueBG <= endvalueBG; startvalueBG++) {
    bg.add(rb[startvalueBG]);
}

b = new JButton("OK");
b.addActionListener(this);
chooseClubPanel.add(b);

quit = new JButton("Quit");
quit.addActionListener(this);
chooseClubPanel.add(quit);

Then you add each button to the frame, but, the frame is using a BorderLayout, so only one button will remain visible, the last one you added...

int startValueAddRB;
int endValueAddRB = 19;
for (startValueAddRB = 0; startValueAddRB <= endValueAddRB; startValueAddRB++) {
    f3.add(rb[startValueAddRB]);
}

Then you try and add the buttons to chooseClubPanel, effectively removing the from the frame, but the indices are all messed up, you are creating 19x19 buttons, each club is assigned to all 19 and the effectively replace by the next club...!?!?

int startRB;
int endRB = 19;

for (startRB = 0; endRB <= 19; startRB++) {
    for (int i = 0; i < premierLeagueClubs.length; i++) {
        rb[startRB] = new JRadioButton(premierLeagueClubs[i]);
        chooseClubPanel.add(rb[i]);
    }
}

The first thing we're going to do is remove the reference to the frame chooseClubPanel, they're just getting in the way and the class has no business generating the frame, that's not it's responsibility.

Next, we're going to make NewGame a JPanel and allow it to become self managing.

Next, we're going to reduce the number of loops from 3 to 1 and do all the work we need to do all at the same time

public class NewGame extends JPanel implements ActionListener {

    private final String[] premierLeagueClubs = {"Arsenal", "Bournemouth", "Burnley", "Chelsea", "Crystal Palace",
                                                                                             "Everton", "Hull City", "Leicester City", "Liverpool", "Manchester United", "Manchester City", "Middlesborough",
                                                                                             "Southampton", "Stoke City", "Sunderland", "Swansea City", "Tottenham Hotspur", "Watford", "West Brom", "West Ham"};
    private final JRadioButton[] rb = new JRadioButton[20];
    private final JButton b, quit;
    String teamName;
    JLabel label1;

    ButtonGroup bg = new ButtonGroup();

    public NewGame() {
        setBackground(Color.GREEN);

        label1 = new JLabel("Please choose a team");
        add(label1);

        for (int i = 0; i < premierLeagueClubs.length; i++) {
            rb[i] = new JRadioButton(premierLeagueClubs[i]);
            rb[i].setActionCommand(premierLeagueClubs[i]);
            bg.add(rb[i]);
            add(rb[i]);
        }

        b = new JButton("OK");
        b.addActionListener(this);
        add(b);

        quit = new JButton("Quit");
        quit.addActionListener(this);
        add(quit);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        System.out.println("You have selected " + bg.getSelection().getActionCommand());
    }

}

At some point you're going to want to display, the simplest solution is to just add it whatever container you want, like a JFrame

public static void main(String[] args) {
    SwingUtilities.invokeLater(new Runnable() {
        @Override
        public void run() {
            JFrame frame = new JFrame("Title");
            frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
            frame.add(new NewGame());
            frame.pack();
            frame.setLocationRelativeTo(null);
            frame.setVisible(true);
        }
    });
}

Normally I suggest having a look at Laying Out Components Within a Container but in your case, I think it would be more pratical to have a look at How to Use Lists