Bejeweled game - performance and code redundancy

Question

I'm fairly new to Java and tried to recreate a Bejeweled game, what I've coded so far works as intended, but I'm not sure if my approach is smart. I set up the board with new gems, but by doing this randomly, patterns (multiple gems of the same colour next to each other) can occur. That's why I recreate the board if there's a pattern. This means that if someone is fairly unlucky this could take a long time to forever.

I also check for patterns after the user has made a move. It only checks for patterns in straight lines, no corners. The checking already has a lot of lines, so I guess this is not really OO or good practice. Is there a better tactic/strategy for such problems? Does it have a name? Will learning about design patterns help me with such problems? Can you help me find sources that can teach me this, since I do not really know what to search for.

In my checkPattern method (is that the correct name, or is it function?) I use "dummy" gems to store gems that are on the grid, is this bad practice?

Are there other things in my code that should be done differently in general?

Bejeweled.java

package Bejeweled;
import javax.swing.*; 
public class Bejeweled extends JFrame{

    public Bejeweled(){
        Board board = new Board();
        getContentPane().add(board);
        board.start();

        setTitle("Bejeweled");
        setSize(400, 400);
        setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    }

    public static void main(String[] args){
        Bejeweled game = new Bejeweled();
        game.setLocationRelativeTo(null);
        game.setVisible(true);
    }
}

Board.java

package Bejeweled;
import javax.swing.*;
import java.awt.*;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
public class Board extends JPanel{

    final int BOARDWIDTH = 8;
    final int BOARDHEIGHT = 8;
    boolean isAlive, isPattern, switchedBack;
    Gem[][] gems;
    int fromX, fromY, toX, toY;
    boolean selected = true;

    public Board() {
        gems = new Gem[BOARDWIDTH][BOARDHEIGHT];

        addMouseListener(new MouseInputAdapter());
    }

    int cellWidth() {
        return (int) getSize().getWidth() / BOARDWIDTH;
    }

    int cellHeight() {
        return (int) getSize().getHeight() / BOARDHEIGHT;
    }

    public void start(){
        isPattern = switchedBack = false;
        isAlive = true;
        do { //create a new grid of cells; if a pattern exists after creating it, do it again
            fillBoard();
            checkPattern();
        }while(isPattern);
    }

    public void paint(Graphics g) {
        super.paint(g);

        for (int x = 0; x < BOARDWIDTH; x++) {
            for (int y = 0; y < BOARDHEIGHT; y++) {
                drawCell(g, x, y, gems[x][y]); //paint a single cell
            }
        }
    }

    public void fillBoard(){ //fill an array of Gem with new gems with random values for 'type'
        for (int x = 0; x < BOARDWIDTH; x++) {
            for (int y = 0; y < BOARDHEIGHT; y++) {
                gems[x][y] = new Gem();
            }
        }
    }

    public void drawCell(Graphics g, int x, int y, Gem gem) {
        x = x * cellWidth();
        y = y * cellHeight();

        Color colors[] = { new Color(255, 0, 0), new Color(255, 128, 0), new Color(255, 255, 0), new Color(0, 255, 0), new Color(0, 255, 255), new Color(0, 0, 255), new Color(127, 0, 255), new Color(0, 0, 0) };
        Color color = colors[gem.getType()];

        g.setColor(color);
        g.fillRect(x, y, x + cellWidth(), y + cellHeight());
        System.out.println(cellWidth());

        g.setColor(Color.BLACK); //border to cells, purely aesthetic
        g.drawLine(x, y, x, y + cellHeight());
        g.drawLine(x, y + cellHeight(), x + cellWidth(), y + cellHeight());
        g.drawLine(x + cellWidth(), y + cellHeight(), x + cellWidth(), y);
        g.drawLine(x + cellWidth(), y, x, y);

    }

    class MouseInputAdapter extends MouseAdapter {
            @Override
            public void mouseClicked(MouseEvent e) {
            selectGems(e);
        }
    }

    public void selectGems(MouseEvent e){
        int x = e.getX() / cellWidth();
        int y = e.getY() / cellHeight();
        if(!selected) { //select the first gem
            fromX = x;
            fromY = y;
            selected = true;
        }else{ //select the second gem and switch them
            toX = x;
            toY = y;
            if((Math.abs(fromX - toX) == 1 ^ Math.abs(fromY - toY) == 1) & (gems[fromX][fromY].getType() != gems[toX][toY].getType())) {
                switchGems();
                selected = false;
            }
        }
    }

    public void switchGems(){ //switch the two gems that the user selected
        int tempType = gems[fromX][fromY].getType();
        gems[fromX][fromY].setType(gems[toX][toY].getType());
        gems[toX][toY].setType(tempType);
        checkPattern(); //check if a pattern has been created because of this change on the board, if not, the gems will be switched back

        switchedBack = false;
        repaint();
    }

    public void checkPattern() {
        Gem dummy1 = new Gem(); //create 2 new Gems that will hold gems that need to be deleted
        Gem dummy2 = new Gem();

        isPattern = false;
        for (int x = 0; x < BOARDWIDTH + 1; x++) {
            for (int y = 0; y < BOARDHEIGHT + 1; y++) {
                try {
                    if ((gems[x][y].getType() == gems[x + 1][y].getType()) && (gems[x + 1][y].getType() == gems[x + 2][y].getType())) { //Checks for 3 horizontal gems in a row
                        try {
                            if (gems[x + 2][y].getType() == gems[x + 3][y].getType()) { //checks for 4 horizontal gems in a row
                                dummy1 = gems[x + 3][y]; //set dummy1 to the fourth gem in the row
                                try {
                                    if (gems[x + 3][y].getType() == gems[x + 4][y].getType()) { //checks for 5 horizontal gems in a row
                                        dummy2 = gems[x + 4][y]; //set dummy2 to the fifth gem in the row
                                    }
                                }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                            }
                        }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                        isPattern = true;
                        deletePattern(gems[x][y], gems[x + 1][y], gems[x + 2][y], dummy1, dummy2); //delete the gems that are in a pattern
                    }
                } catch (ArrayIndexOutOfBoundsException e) {e.printStackTrace();}
                try{
                    if ((gems[x][y].getType() == gems[x][y + 1].getType()) && (gems[x][y + 1].getType() == gems[x][y + 2].getType())) { //Check for 3 vertical gems in a row
                        try {
                            if (gems[x][y + 2].getType() == gems[x][y + 3].getType()) { //checks for 4 vertical gems in a row
                                dummy1 = gems[x][y + 3]; //set dummy1 to the fourth gem in the row
                                try {
                                    if (gems[x][y + 3].getType() == gems[x][y + 4].getType()) { //check for 5 vertical gems in a row
                                        dummy2 = gems[x][y + 4]; //set dummy2 to the fourth gem in the row
                                    }
                                }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                            }
                        }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                        isPattern = true;
                        deletePattern(gems[x][y], gems[x][y + 1], gems[x][y + 2], dummy1, dummy2); //delte the gems that are in a pattern
                    }
                } catch (ArrayIndexOutOfBoundsException e) {e.printStackTrace();}
            }
        }

        if(!isPattern && !switchedBack){ //if there's no pattern and not switchedback, meaning it hasn't already run the code below before during this move: execute it.
            switchedBack = true;
            switchGems(); //Switching the gems didn't make a pattern so this kind of resets it
        }
    }

    public void deletePattern(Gem gem1, Gem gem2, Gem gem3, Gem gem4, Gem gem5){
        gem1.setType(7);
        gem2.setType(7);
        gem3.setType(7);
        gem4.setType(7);
        gem5.setType(7);
        for (int x = 0; x < BOARDWIDTH; x++) {
            for (int y = 0; y < BOARDHEIGHT; y++) {
                if(gems[x][y].getType() == 7){ //search for all gems with type 7 indicating it should be deleted
                    for (int i = y; i >= 0; i--) {
                        if(i == 0) //if a gem on the top row is deleted, generate a new one
                            gems[x][i].setType(gems[x][i].genType());
                        else //if some other gem is deleted copy the one above it to its current position
                            gems[x][i].setType(gems[x][i-1].getType());
                    }
                }
            }
        }
        checkPattern(); //check for newly created patterns because of changes on the board. This means it will do this until there are no more patterns.
    }
}

Gem.java

package Bejeweled;
public class Gem {

    private int type;
    public Gem(){
        this.type = genType();
    }

    public int genType(){ //generate a random value for 'type' corresponding its colour
        return (int) (Math.random() * 7);
    }

    public void setType(int type){
        this.type = type;
    }

    public int getType(){
        return type;
    }
}

Answer 1

So, performance, and Swing, and Println..... there's the problem.

println

System.out.println() is a great debug tool, but it is shocking for performance. There are a few things that go against it....

1. it is synchronized (so you have to do a whole bunch of memory management)
2. it does IO (accessing buffers and other OS resources).
3. it flushes the streams
4. it does string manipulation

So, your code, in the most performance critical part here:

public void drawCell(Graphics g, int x, int y, Gem gem) {
    x = x * cellWidth();
    y = y * cellHeight();

    ....

    System.out.println(cellWidth());

    ....

}

will be shockingly slow.

constants

In that same method, you have:

    Color colors[] = { new Color(255, 0, 0), new Color(255, 128, 0), new Color(255, 255, 0), new Color(0, 255, 0), new Color(0, 255, 255), new Color(0, 0, 255), new Color(127, 0, 255), new Color(0, 0, 0) };
    Color color = colors[gem.getType()];

That whole array is a lot of work to create, just to throw it away again. You can make that a constant:

private static final Color COLORS[] = { new Color(255, 0, 0), 
        new Color(255, 128, 0), new Color(255, 255, 0),
        new Color(0, 255, 0), new Color(0, 255, 255),
        new Color(0, 0, 255), new Color(127, 0, 255),
        new Color(0, 0, 0) };

Then, in your method just have:

    Color color = COLORS[gem.getType()];

There are a few other places this type of strategy would make a difference, find them, and fix them, and compare the performance difference.

Exceptions ...

oh, dear.... here's one I missed because.... I really was not expecting it.

Using exceptions as part of the normal program flow is considered to be really bad practice. Some languages have very efficient exception handling. Java is very efficient, but exceptions still do a huge amount of work.

So, code like this is horrible, horribl, horrible:

    for (int x = 0; x < BOARDWIDTH + 1; x++) {
        for (int y = 0; y < BOARDHEIGHT + 1; y++) {
            try {
                if ((gems[x][y].getType() == gems[x + 1][y].getType()) && (gems[x + 1][y].getType() == gems[x + 2][y].getType())) { //Checks for 3 horizontal gems in a row
                    try {
                        if (gems[x + 2][y].getType() == gems[x + 3][y].getType()) { //checks for 4 horizontal gems in a row
                            dummy1 = gems[x + 3][y]; //set dummy1 to the fourth gem in the row
                            try {
                                if (gems[x + 3][y].getType() == gems[x + 4][y].getType()) { //checks for 5 horizontal gems in a row
                                    dummy2 = gems[x + 4][y]; //set dummy2 to the fifth gem in the row
                                }
                            }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                        }
                    }catch(ArrayIndexOutOfBoundsException e){e.printStackTrace();}
                    isPattern = true;
                    deletePattern(gems[x][y], gems[x + 1][y], gems[x + 2][y], dummy1, dummy2); //delete the gems that are in a pattern
                }
            } catch (ArrayIndexOutOfBoundsException e) {e.printStackTrace();}

That loops through each grid position, but instead of 'clipping' the checks using simple conditions, you try/fail them.

Use the following types of logic instead:

    for (int x = 0; x <= BOARDWIDTH; x++) {
        for (int y = 0; y <= BOARDHEIGHT; y++) {
            if (x + 2 < BOARDWIDTH
                && gems[x][y].getType() == gems[x + 1][y].getType()
                && (gems[x + 1][y].getType() == gems[x + 2][y].getType())) {
                if (x + 3 < BOARDWIDTH
                    && gems[x + 2][y].getType() == gems[x + 3][y].getType()) {
                    dummy1 = gems[x + 3][y];

                    .....

Answer 2

Object Orientedness

I would say that your approach is not particularly object oriented. Having created an object oriented bejeweled implementation, I would say that it is definitely not very performant, especially when you start working on the AI. So I would not necessarily worry about how object oriented your approach is, unless you are specifically wanting to do things that way.

Gem Deletion

My biggest problem with the code is this method:

public void deletePattern(Gem gem1, Gem gem2, Gem gem3, Gem gem4, Gem gem5){
    gem1.setType(7);
    gem2.setType(7);
    gem3.setType(7);
    gem4.setType(7);
    gem5.setType(7);
    for (int x = 0; x < BOARDWIDTH; x++) {
        for (int y = 0; y < BOARDHEIGHT; y++) {
            if(gems[x][y].getType() == 7){ //search for all gems with type 7 indicating it should be deleted
                for (int i = y; i >= 0; i--) {
                    if(i == 0) //if a gem on the top row is deleted, generate a new one
                        gems[x][i].setType(gems[x][i].genType());
                    else //if some other gem is deleted copy the one above it to its current position
                        gems[x][i].setType(gems[x][i-1].getType());
                }
            }
        }
    }
    checkPattern(); //check for newly created patterns because of changes on the board. This means it will do this until there are no more patterns.
}

This method indicates a few problems with your approach. Is the maximum number of gems in a combination five? This seems pretty restrictive. Unfortunately this is related to your massive checkPattern() method, since it appears to only check for up to five matches as well.

Beyond this, it feels wrong to be creating dummy objects to send into this method. I assume you are doing this to prevent null pointer exceptions. Instead of doing things this way, I would have the checkPattern() method add the Gem objects into an array, and then pass the array into the deletePattern() method.

Separating Model and View

You have all of your game logic, rendering logic, and input logic in one class, the Board class. I would highly recommend removing all of the rendering and input logic from this class, and placing them in a View class. The View class would handle input from the player, apply the logic to the Board (or Game if you add one of those later), and then render the result to the screen.

It is a good idea to get into the habit of doing this separation whenever possible. That way if you later decide to do something like add an AI player, you will only have to change things slightly to receive commands from the AI instead of the player. As it is now, it would require rewriting almost everything to extend the functionality in this way.

Random

I have seen many recommendations to use Random.nextInt() instead of Math.random(). Here is a detailed explanation of why Random.nextInt() is better.

Answer 3

I want to talk about your basic, the Gem class because the others didn't mentioned it.

But a model is the basics, and when something goes wrong there you feel it later on.

What does the Gem contain? Nothing useful! In fact, with this code your Gem you can be refactored to an Integer.

Before we start, let's think a little ahead.

1. Different types.

You want to have x kinds of gems. The gems will not be loaded from a DB I guess but hard-coded in your game. With this on my mind, I'm thinking that the gems are kind a of state. In my opinion, the best way to have different states is an enum. Let's hold that thought while we think further.

2. Does the gem have specialties?

Yes, it does. Why should you declare a color in the GUI as you can put the color in your Gem object? With an enum we can declare the color also when instantiating. Maybe in the future you want to add additional things like shape, points, special power, ... Again, it's at the place and easy to refactor.

A great example of enum usage you find at the bottom. (the planets)

3. Equation.

We have to do a lot of equations. enum could be checked with == and is even null safe.

4. Random value.

We need to have a random value. This is also possible. I'll just give some heads up with that code (this didn't include the first point):

public enum Gem{
  GREEN,
  RED,
  DIAMOND,
  BOMB,
  ...

  private static final List<Gem> VALUES = Collections.unmodifiableList(Arrays.asList(values()));
  private static final int SIZE = VALUES.size();
  private static final Random RANDOM = new Random();

  public static Gem randomGem(final double difficulty)  {
    if (difficulty > 1.0 || difficulty < 0.1) {
        throw new AssertionError("Difficulty could not be higher then 1 or lower then 0.1.");
    } 
    return VALUES.get(RANDOM.nextInt((int) SIZE * difficulty));
  }
}

What do you see? Indeed, I even have implemented a kind of difficulty. If you want to create a simpler game, give an double of 0.5 and your first half of gems will be used.

I hope this is useful to you.