Basic Memory Match Game in Java

I'm relatively new to Java and decided to make a Memory Match game.

Card.java:

import javax.swing.JButton;

@SuppressWarnings("serial")
public class Card extends JButton{
    private int id;
    private boolean matched = false;

    public void setId(int id){
        this.id = id;
    }

    public int getId(){
        return this.id;
    }

    public void setMatched(boolean matched){
        this.matched = matched;
    }

    public boolean getMatched(){
        return this.matched;
    }
}

Board.java:

```
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import javax.swing.Timer;
import java.awt.*;
import java.awt.event.*;
import java.util.ArrayList;
import java.util.List;
import java.util.Collections;

@SuppressWarnings("serial")
public class Board extends JFrame{

private List cards;
private Card selectedCard;
private Card c1;
private Card c2;
private Timer t;

public Board(){

int pairs = 10;
List cardsList = new ArrayList();
List cardVals = new ArrayList();

for (int i = 0; i < pairs; i++){
cardVals.add(i);
cardVals.add(i);
}
Collections.shuffle(cardVals);

for (int val : cardVals){
Card c = new Card();
c.setId(val);
c.addActionListener(new ActionListener(){
public void actionPerformed(ActionEvent ae){
selectedCard = c;
doTurn();
}
});
cardsList.add(c);
}
this.cards = cardsList;
//set up the timer
t = new Timer(750, new ActionListener(){
public void actionPerformed(ActionEvent ae){
checkCards();
}
});

t.setRepeats(false);

//set up the board itself
Container pane = getContentPane();
pane.setLayout(new GridLayout(4, 5));
for (Card c : cards){
pane.add(c);
}
setT

Card class

Your id does not change after being set once. This makes it perfect for being set inside a constructor. This will also allow you to make it private final int id; . It is a good practice to use final fields whenever possible.

Instead of:

Card c = new Card();
c.setId(val);

Do:

Card c = new Card(val);

Board

selectedCard is only a temporarily-used variable which is set in one method and read directly afterwards in another. It should be passed on to the doTurn method, no need to use it as a class field.

Instead of:

selectedCard = c;
doTurn();

Do:

doTurn(c);

doTurn

As suggested above, this method should take the selectedCard parameter directly.

At the moment, this method does a little bit too much and contains some slight code duplication.

c1.setText(String.valueOf(c1.getId()));
c2.setText(String.valueOf(c2.getId()));

There's no fundamental difference between those two lines, and if asked what those lines do you would probably say something like "it shows the text on the cards". Now let's make that a method in the Card class!

public void showText() {
      this.setText(String.valueOf(id));
 }

Your doTurn method then becomes:

public void doTurn(Card selectedCard) {
    if (c1 == null && c2 == null) {
        c1 = selectedCard;
        c1.showText();
    }
    else if (c1 != null && c1 != selectedCard && c2 == null) {
        c2 = selectedCard;
        c2.showText();
        t.start();
    }
}

checkCards

if (c1.getId() == c2.getId()){//match condition

Can be extracted to a method and called like:

if (c1.hasSameId(c2)) {

c1.setEnabled(false); //disables the button
c2.setEnabled(false);
c1.setMatched(true); //flags the button as having been matched
c2.setMatched(true);

Code duplication. Can be extracted to c1.matchButton();

System.exit(0);

This is an 'evil' way of exiting your application. It is preferred to close the JFrame instead and shut down the timer. That will exit your application in a more 'friendly' way.

c1.setText(""); //'hides' text
c2.setText("");

Again, method extraction will make this code more self-explanatory, and make the buttons better be responsible for what they should be responsible for.

c1.hideText();
c2.hideText();

Game Won and Streams

In Java 8 (which I believe that you are using) your isGameWon method can be simplified:

public boolean isGameWon(){
    return this.cards.stream().allMatch(c -> c.isMatched());
}

Composition over inheritance

This goes back to the Card class primarily:

public class Card extends JButton

With the above suggestions of extracting methods to make the Card class be responsible for setting text and disabling the buttons and stuff, you can remove the extends JButton and write it more like this:

public class Card {
     private final int id;
     private final Button button = new Button();
     ...

     public Card(int id) {
          this.id = id;
     }

     public Button getButton() {
          return this.button;
     }

     ...
}

And create it like this:

Card c = new Card(val);
c.setId(val);
c.getButton().addActionListener(new ActionListener(){
    public void actionPerformed(ActionEvent ae){
        selectedCard = c;
        doTurn();
    }
});

The extra step

This is a slightly more advanced concept. Use it at your own risk, if you feel that you can understand it.

Side note: It is possible to make this even better, by for example pass on a Consumer to the Card class and pass on this::doTurn when constructing the cards, and have your Card action listener call this Consumer.

On the positive side, this will remove the need of exposing the Button in your Card class. On the negative side, this might be a bit "over your head" at the moment.

Card c = new Card(val, this::doTurn);

public class Card {
     private final int id;
     private final Button button = new Button();
     ...

     public Card(int id, Consumer onClick) {
          this.id = id;
          this.button.addActionListener(new ActionListener() {
              public void actionPerformed(ActionEvent ae) {
                  onClick.accept(Card.this);
              }
          });
     }

     public Button getButton() {
          return this.button;
     }

     ...
}

StackExchange Code Review Q#85833, answer score: 8