Take the 2-minute tour ×

I'm writing a clone of Risk in Java, and am having some trouble with my code. When I create a new game, I create a JPanel with a JTextField (for the player name) and a JComboBox(for the player color), one panel for each player the user wants to create. Instances of this panel are created dynamically based on a second JComboBox which lets the user select a number of players from three to eight.

My problem is that when I want to create the player objects from the data entered into the player creation panel, each player object retrieves the data from the most-recently-created player creation panel. I have a functional solution, but I can't seem to figure out why what seems to be the 'proper' solution () won't work.

This is the code that I have working right now: Creation panel class:

package risk;

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

import javax.swing.ImageIcon;
import javax.swing.JComboBox;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;

public class PlayerCreatePanel extends JPanel{

public static ImageIcon[] playerColors = {Resources.red,Resources.green,Resources.blue,Resources.cyan,Resources.magenta,Resources.yellow,Resources.orange,Resources.gray};

public JComboBox playerColor;

public JTextField playerName;

public PlayerCreatePanel(int index){

    this.setPreferredSize(new Dimension(360, 30));

    JLabel numberLabel = new JLabel ("Player " + index + ":  ");
    JLabel nameLabel = new JLabel("Name: ");
    JLabel colorLabel = new JLabel("  Color: ");

    playerName = new JTextField("");

    playerColor = new JComboBox(playerColors);


And for the new game class, the part that creates players:

package risk;

import java.awt.Color;
import java.awt.Dimension;
import javax.swing.JPanel;

public class NewGame {

private static PlayerCreatePanel [] panels = {  new PlayerCreatePanel(1), new PlayerCreatePanel(2), 
                                                new PlayerCreatePanel(3), null, null, null, null, null};

private static int playerCount = 3;

public static void createPlayers(){
    Resources.players = new Player[playerCount];
    case 8: Resources.players[7] = new Player (panels[7].playerName.getText(), Resources.colors[panels[7].playerColor.getSelectedIndex()], 8);
    case 7: Resources.players[6] = new Player (panels[6].playerName.getText(), Resources.colors[panels[6].playerColor.getSelectedIndex()], 7);
    case 6: Resources.players[5] = new Player (panels[5].playerName.getText(), Resources.colors[panels[5].playerColor.getSelectedIndex()], 6);
    case 5: Resources.players[4] = new Player (panels[4].playerName.getText(), Resources.colors[panels[4].playerColor.getSelectedIndex()], 5);
    case 4: Resources.players[3] = new Player (panels[3].playerName.getText(), Resources.colors[panels[3].playerColor.getSelectedIndex()], 4);
    case 3: Resources.players[2] = new Player (panels[2].playerName.getText(), Resources.colors[panels[2].playerColor.getSelectedIndex()], 3);
            Resources.players[1] = new Player (panels[1].playerName.getText(), Resources.colors[panels[1].playerColor.getSelectedIndex()], 2);
            Resources.players[0] = new Player (panels[0].playerName.getText(), Resources.colors[panels[0].playerColor.getSelectedIndex()], 1); break;
    default: break;     

Now, what I've learned is the proper way to do this is to make my JTextField and JComboBox private, and write accessors, like so:

private JComboBox playerColor;
private JTextField playerName;

//...same method as above

public static String getName(){
    return playerName.getText();

public static int getColorIndex(){
    return playerColor.getSelectedIndex();

and change the creation lines in the new game method to read something like this:

Resources.players [0] = new Player (panels[0].getName(), panels[0].getColorIndex(), 1);
share|improve this question
You probably have a mistake somewhere else. Your code is not very readable, but I can't see, where information can be shared between panels. Probably you mix zero- and one- based numbering somewhere. –  Suzan Cioc Oct 4 '12 at 17:32
Note that you are loosing index while creating panel. It is kept only in string form within numberLabel. You would better use it for indexing. Also you shoul better use for loop instead of switch. –  Suzan Cioc Oct 4 '12 at 17:35
You're right, I should be using a for loop -- the switch statement is something I kept from an older version of the program, and it should be switched. Mixing numbering isn't the problem because that would cause a different problem than I am having (variables would be shifted, rather than all set to the same value), right? Index is preserved as the index number in the array of panels, I don't need it for anything else, do I? If you want to see the full code, I can post that too, I was just trying not to flood... –  user18 Oct 4 '12 at 17:38
add comment


Your Answer



Browse other questions tagged or ask your own question.