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

Bejeweled game - performance and code redundancy

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

Problem

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 = t

Solution

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 = 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.

Code Snippets

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.
}

Context

StackExchange Code Review Q#90452, answer score: 14

Revisions (0)

No revisions yet.