7
\$\begingroup\$

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<Card> cards;
    private Card selectedCard;
    private Card c1;
    private Card c2;
    private Timer t;

    public Board(){

        int pairs = 10;
        List<Card> cardsList = new ArrayList<Card>();
        List<Integer> cardVals = new ArrayList<Integer>();

        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);
        }
        setTitle("Memory Match");
    }

    public void doTurn(){
        if (c1 == null && c2 == null){
            c1 = selectedCard;
            c1.setText(String.valueOf(c1.getId()));
        }

        if (c1 != null && c1 != selectedCard && c2 == null){
            c2 = selectedCard;
            c2.setText(String.valueOf(c2.getId()));
            t.start();

        }
    }

    public void checkCards(){
        if (c1.getId() == c2.getId()){//match condition
            c1.setEnabled(false); //disables the button
            c2.setEnabled(false);
            c1.setMatched(true); //flags the button as having been matched
            c2.setMatched(true);
            if (this.isGameWon()){
                JOptionPane.showMessageDialog(this, "You win!");
                System.exit(0);
            }
        }

        else{
            c1.setText(""); //'hides' text
            c2.setText("");
        }
        c1 = null; //reset c1 and c2
        c2 = null;
    }

    public boolean isGameWon(){
        for(Card c: this.cards){
            if (c.getMatched() == false){
                return false;
            }
        }
        return true;
    }

}

Game.java:

import java.awt.Dimension;
import javax.swing.JFrame;


public class Game{
    public static void main(String[] args){
        Board b = new Board();
        b.setPreferredSize(new Dimension(500,500)); //need to use this instead of setSize
        b.setLocation(500, 250);
        b.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        b.pack();
        b.setVisible(true);
    }   
}

The code compiles and the game works, which is fine, but how could I improve, if at all, from a stylistic perspective? If it helps, before Java I worked in primarily Python and C.

\$\endgroup\$
0

1 Answer 1

8
\$\begingroup\$

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<Card> to the Card class and pass on this::doTurn when constructing the cards, and have your Card action listener call this Consumer<Card>.

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<Card> onClick) {
          this.id = id;
          this.button.addActionListener(new ActionListener() {
              public void actionPerformed(ActionEvent ae) {
                  onClick.accept(Card.this);
              }
          });
     }

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

     ...
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.