HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Classic two-player memory game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
playerclassictwogamememory

Problem

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 list1 = new ArrayList();
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.se

Solution

I would have some suggestions on the code

  • Make your fields private.



  • Do not specify the implementation class, as in ArrayList list1 = new ArrayList();. Use List list1 = new ArrayList(); 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 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 buttons = Arrays.asList(new JButton[NUM_OF_BUTTONS]);. Then for looping all the buttons, say for(int i = 0; i



  • 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

Context

StackExchange Code Review Q#99214, answer score: 4

Revisions (0)

No revisions yet.