6
\$\begingroup\$

This is a classic memory game with a points counter for the two players.

The app works fine, but since this is my first project in Swing, I would appreciate the critical opinion of some expert, as I'm sure there is plenty of space for code improvement/optimization.

What do you think about the code? What should have I done in a different/better way? What are your recommendations in terms of optimization?

public class Pixeso extends JFrame {

    private JPanel contentPane;
    ImageIcon[] iconarray;
    ArrayList<Integer> list1 = new ArrayList<Integer>();
    JButton[] buttonarray = new JButton[20];
    Random rand1 = new Random();
    JButton button1;
    JButton button2;
    int counter = 1;
    Timer timer1;
    int points1;
    int points2;
    boolean player1 = true;
    private final JPanel panel1 = new JPanel();
    private final JPanel panel2 = new JPanel();
    private final JLabel label1 = new JLabel("0");
    private final JLabel label2 = new JLabel("0");
    private final JLabel lblNewLabel = new JLabel("Player 1");
    private final JLabel lblNewLabel_1 = new JLabel("Player 2");

    /**
     * Launch the application.
     */
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    Pixeso frame = new Pixeso();
                    frame.setVisible(true);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        });
    }

    /**
     * Create the frame.
     * 
     * @throws IOException
     */
    public Pixeso() throws IOException {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setBounds(100, 100, 1204, 908);
        contentPane = new JPanel();
        contentPane.setBackground(Color.GREEN);
        contentPane.setBorder(new EmptyBorder(0, 0, 0, 0));
        setContentPane(contentPane);
        contentPane.setLayout(new FlowLayout(FlowLayout.CENTER, 5, 5));
        panel1.setBackground(Color.GREEN);

        contentPane.add(panel1);
        panel1.setLayout(new GridLayout(5, 4, 5, 5));
        panel2.setBorder(new LineBorder(new Color(0, 0, 0)));

        contentPane.add(panel2);
        lblNewLabel.setHorizontalAlignment(SwingConstants.CENTER);
        lblNewLabel.setVerticalAlignment(SwingConstants.TOP);
        lblNewLabel.setFont(new Font("Tahoma", Font.PLAIN, 16));
        lblNewLabel.setBackground(Color.WHITE);
        label1.setHorizontalAlignment(SwingConstants.CENTER);
        label1.setVerticalAlignment(SwingConstants.TOP);
        label1.setForeground(Color.RED);
        label1.setFont(new Font("Tahoma", Font.PLAIN, 16));
        label1.setBackground(Color.WHITE);
        label2.setHorizontalAlignment(SwingConstants.CENTER);
        label2.setForeground(Color.RED);
        label2.setVerticalAlignment(SwingConstants.TOP);
        label2.setFont(new Font("Tahoma", Font.PLAIN, 16));
        label2.setBackground(Color.WHITE);
        panel2.setLayout(new GridLayout(2, 2, 20, 10));
        panel2.add(lblNewLabel);
        panel2.add(label1);
        lblNewLabel_1.setHorizontalAlignment(SwingConstants.CENTER);
        lblNewLabel_1.setVerticalAlignment(SwingConstants.TOP);
        lblNewLabel_1.setFont(new Font("Tahoma", Font.PLAIN, 16));
        lblNewLabel_1.setBackground(Color.WHITE);
        panel2.add(lblNewLabel_1);
        panel2.add(label2);

        // put the imagines in a URL[]
        URL[] immagini = new URL [11];
    immagini[0] = new URL("http://i.imgur.com/421DcmK.jpg");
    immagini[1] = new URL("http://i.imgur.com/mpx0yXN.jpg");
    immagini[2] = new URL("http://i.imgur.com/9i8UkrI.jpg");
    immagini[3] = new URL("http://i.imgur.com/KN86BKv.jpg");
    immagini[4] = new URL("http://i.imgur.com/KN86BKv.jpg");
    immagini[5] = new URL("http://i.imgur.com/mS3dRj7.jpg");
    immagini[10] = new URL("http://i.imgur.com/7vdVgHa.jpg");
    immagini[7] = new URL("http://i.imgur.com/njAuT7Q.jpg");
    immagini[8] = new URL("http://i.imgur.com/5hWZQG8.jpg");
    immagini[9] = new URL("http://i.imgur.com/bwZAiyL.jpg");
    immagini[6] = new URL("http://i.imgur.com/rHbAnOD.jpg");

        iconarray = new ImageIcon[11];
        // convert imagines in icons
        for (int i = 0; i <= 10; i++) {
            iconarray[i] = new ImageIcon(immagini[i]);
            // inizializzo list1
            list1.add(0);
        }

        // add 20 JButtons to panel 1 and set initial icon
        for (int i = 0; i < 20; i++) {
            panel1.add(new JButton(iconarray[10]));
            // insert JButtons in buttonarray
            buttonarray[i] = (JButton) panel1.getComponent(i);
            // add ImageButtonListener method to each JButton
            buttonarray[i].addActionListener(new ImageButtonListener());

        }

        // add a number between 0 and 9 for each JButton
        int y = 0;
        while (y < 20) {
            int x = rand1.nextInt(10);
            list1.set(x, list1.get(x).intValue() + 1);
            if (list1.get(x) <= 2) {
                buttonarray[y].setName(Integer.toString(x));
                y++;
            }
        }

        timer1 = new Timer(2000, new TimerListener());

    }

    // this timer show clicked cards for two seconds
    private class TimerListener implements ActionListener {
        public void actionPerformed(ActionEvent e) {
            button1.setIcon(iconarray[10]);
            button2.setIcon(iconarray[10]);
            timer1.stop();
            // active = true;
        }
    }

    // method to change JButton image
    class ImageButtonListener implements ActionListener {
        public void actionPerformed(ActionEvent e) {
            // waiting for timer to pop, user clicks not accepted
            if (timer1.isRunning())
                return;

            for (int i = 0; i < 20; i++)
                if (e.getSource() == buttonarray[i]) {
                    int x = Integer.parseInt(buttonarray[i].getName());
                    buttonarray[i].setIcon(iconarray[x]);

                    // button1=  first clicked button
                    if (counter == 1) {
                        button1 = buttonarray[i];
                        counter++;
                    }
                    // button 2= second clicked button, check I didn't click same card twice
                if (counter == 2 && buttonarray[i] != button1) {
                        button2 = buttonarray[i];
                        compareicons();
                    }
                }
        }

        // check if icons match
        private void compareicons() {
            if (button1.getIcon() == button2.getIcon()) {
                button1.setEnabled(false);
                button2.setEnabled(false);

        //add up points to player who found two matching icons
                if (player1 == true) {
                    points1++;
                    label1.setText(Integer.toString(points1));
                } else {
                    points2++;
                    label2.setText(Integer.toString(points2));
                }
            }

        //if cards are different, switch to other player
            else {
                if (player1 == true) {
                    player1 = false;
                } else {
                    player1 = true;
                }
                timer1.start();
            }
            //reset counter
            counter = 1;
        }
    }

}
\$\endgroup\$
4
\$\begingroup\$

I would have some suggestions on the code

  • Make your fields private.
  • Do not specify the implementation class, as in ArrayList<Integer> list1 = new ArrayList<Integer>();. Use List<Integer> list1 = new ArrayList<Integer>(); instead.
  • Either initialize the fields in declaration or in constructor, don't mix both approach. I personally would prefer consistency in code.
  • Give more meaningful names to variables, especially fields. Say what is button1 and button2? points1 and points2?
  • In general use Lists instead of arrays directly even if it is fixed size. It's pretty legacy. In substitute I would use List<JButton> buttons = Arrays.asList(new JButton[20]);, and speaking of which
  • Avoid hard-coding the number of things as literal. Especially in for-loop. Trouble ensues when the length changes. Use a constant like private static final int NUM_OF_BUTTONS = 20; and initialize the buttons as List<JButton> buttons = Arrays.asList(new JButton[NUM_OF_BUTTONS]);. Then for looping all the buttons, say for(int i = 0; i < buttons.size(); i++), or even better for(JButton button : buttons). In general, I would do that whenever I see I use the same literal twice.
  • Avoid superfluous boolean equality checking or assignment. E.g. if (player1 == true) can be replaced by if(player1), if (player1 == true) { player1 = false; } else { player1 = true; } can be replaced by player1 = !player1;

That's it on top of my head

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your suggestions, but can you please develop points 2 and 5? What's the advantage in not specifying the implementation class and in using lists instead of arrays? \$\endgroup\$ – Alex Reds Aug 19 '15 at 15:08
0
\$\begingroup\$

After re-working my code following Edwin's advice, I would liketo split it in 4 different classes (the entire code above sits withing the Pixeso class): the main class, a JFrame constructors class, a TimerListener event handler class and an ImageButtonListener event handler class

My only doubt is if it is a good idea to decouple the event handlers from the class where the frame is built, since my first attempt to do that was unsuccessful - maybe it's a better idea to leave them within the class with the constructor?

\$\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.