patternjavaMinor
Classic two-player memory game
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
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
That's it on top of my head
- Make your fields private.
- Do not specify the implementation class, as in
ArrayList list1 = new ArrayList();. UseList 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 asList buttons = Arrays.asList(new JButton[NUM_OF_BUTTONS]);. Then for looping all the buttons, sayfor(int i = 0; i
- Avoid superfluous boolean equality checking or assignment. E.g. if (player1 == true)
can be replaced byif(player1),if (player1 == true) { player1 = false; } else { player1 = true; }can be replaced byplayer1 = !player1;`
That's it on top of my head
Context
StackExchange Code Review Q#99214, answer score: 4
Revisions (0)
No revisions yet.