13
\$\begingroup\$

I am a self - taught coder, and have been learning Java / C# for the last 2 1/2 years (inconsistently).

I have recently created a snake game in Java with the help of an online tutorial. I have rearranged the program into separate classes & methods. I am skeptical on how good of a programmer I am, and I code messy.

I don't really want help on how to make the game better in visuals, levels, or anything like that. I am asking those who are more advanced programmers to help me with implementing programming techniques, better organization, how to comment better, etc. Pretty much anything that will help me become a better programmer.

Also, I have commented a few things out of pure guess, so if it appears as if I have gotten something wrong, please correct me!

Also, any extra tips for my future projects would be much appreciated!

Game.Java

import java.awt.EventQueue;

import javax.swing.JFrame;

public class Game extends JFrame {

Game() {
    add(new Board());
    setResizable(false);
    pack();

    setTitle("Snake");
    setLocationRelativeTo(null);
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

public static void main(String[] args) {

    // Creates a new thread so our GUI can process itself
    EventQueue.invokeLater(new Runnable() {
        @Override
        public void run() {
            JFrame frame = new Game();
            frame.setVisible(true);
        }
    });
}
}

Snake.Java

public class Snake {

// Stores the joints / body part locations for our snake
private final int[] x = new int[Board.getAllDots()];
private final int[] y = new int[Board.getAllDots()];

// Stores direction of our snake
private boolean movingLeft = false;
private boolean movingRight = false;
private boolean movingUp = false;
private boolean movingDown = false;

private int joints = 0; // Stores # of dots / joints the snake has (starts
                        // with 3)

public int getSnakeX(int index) {
    return x[index];
}

public int getSnakeY(int index) {
    return y[index];
}

public void setSnakeX(int i) {
    x[0] = i;
}

public void setSnakeY(int i) {
    y[0] = i;
}

public boolean isMovingLeft() {
    return movingLeft;
}

public void setMovingLeft(boolean movingLeft) {
    this.movingLeft = movingLeft;
}

public boolean isMovingRight() {
    return movingRight;
}

public void setMovingRight(boolean movingRight) {
    this.movingRight = movingRight;
}

public boolean isMovingUp() {
    return movingUp;
}

public void setMovingUp(boolean movingUp) {
    this.movingUp = movingUp;
}

public boolean isMovingDown() {
    return movingDown;
}

public void setMovingDown(boolean movingDown) {
    this.movingDown = movingDown;
}

public int getJoints() {
    return joints;
}

public void setJoints(int j) {
    joints = j;
}

public void move() {
    for (int i = joints; i > 0; i--) {

        // Moves the joints of the snake 'up the chain'
        // Meaning, the joint of the snake all move up one
        x[i] = x[(i - 1)];
        y[i] = y[(i - 1)];
    }

    // Moves snake to the left
    if (movingLeft) {
        x[0] -= Board.getDotSize();
    }
    // To the right
    if (movingRight) {
        x[0] += Board.getDotSize();
    }
    // Down
    if (movingDown) {
        y[0] += Board.getDotSize();
    }
    // And finally up
    if (movingUp) {
        y[0] -= Board.getDotSize();
    }

    // Dotsize represents the size of the joint, so a pixel of DOTSIZE
    // gets added on to the snake in that direction
}
 }

Food.java

public class Food {

private Snake snake = new Snake();
private int foodX; // Stores X pos of our food
private int foodY; // Stores Y pos of our food

// Used to determine random position of food
private final int RANDOMPOSITION = 40;

public void createFood() {

    // Set our food's x & y position to a random position

    int location = (int) (Math.random() * RANDOMPOSITION);
    foodX = ((location * Board.getDotSize()));

    location = (int) (Math.random() * RANDOMPOSITION);
    foodY = ((location * Board.getDotSize()));

    if ((foodX == snake.getSnakeX(0)) && (foodY == snake.getSnakeY(0))) {
        createFood();
    }
}

public int getFoodX() {

    return foodX;
}

public int getFoodY() {
    return foodY;
}
}

Board.Java

    import java.awt.Color;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.FontMetrics;
import java.awt.Graphics;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyAdapter;
import java.awt.event.KeyEvent;
import javax.swing.JPanel;
import javax.swing.Timer;

@SuppressWarnings("serial")
public class Board extends JPanel implements ActionListener {

// TODO: Implement a way for the player to win

// Holds height and width of the window
private final static int BOARDWIDTH = 1000;
private final static int BOARDHEIGHT = 980;

// Used to represent pixel size of food & our snake's joints
private final static int PIXELSIZE = 25;

// The total amount of pixels the game could possibly have.
// We don't want less, because the game would end prematurely.
// We don't more because there would be no way to let the player win.

private final static int TOTALPIXELS = (BOARDWIDTH * BOARDHEIGHT)
        / (PIXELSIZE * PIXELSIZE);

// Check to see if the game is running
private boolean inGame = true;

// Timer used to record tick times
private Timer timer;

// Used to set game speed, the lower the #, the faster the snake travels
// which in turn
// makes the game harder.
private static int speed = 45;

// Instances of our snake & food so we can use their methods
private Snake snake = new Snake();
private Food food = new Food();

public Board() {

    addKeyListener(new Keys());
    setBackground(Color.BLACK);
    setFocusable(true);

    setPreferredSize(new Dimension(BOARDWIDTH, BOARDHEIGHT));

    initializeGame();
}

// Used to paint our components to the screen
@Override
protected void paintComponent(Graphics g) {
    super.paintComponent(g);

    draw(g);
}

// Draw our Snake & Food (Called on repaint()).
void draw(Graphics g) {
    // Only draw if the game is running / the snake is alive
    if (inGame == true) {
        g.setColor(Color.green);
        g.fillRect(food.getFoodX(), food.getFoodY(), PIXELSIZE, PIXELSIZE); // food

        // Draw our snake.
        for (int i = 0; i < snake.getJoints(); i++) {
            // Snake's head
            if (i == 0) {
                g.setColor(Color.RED);
                g.fillRect(snake.getSnakeX(i), snake.getSnakeY(i),
                        PIXELSIZE, PIXELSIZE);
                // Body of snake
            } else {
                g.fillRect(snake.getSnakeX(i), snake.getSnakeY(i),
                        PIXELSIZE, PIXELSIZE);
            }
        }

        // Sync our graphics together
        Toolkit.getDefaultToolkit().sync();
    } else {
        // If we're not alive, then we end our game
        endGame(g);
    }
}

void initializeGame() {
    snake.setJoints(3); // set our snake's initial size

    // Create our snake's body
    for (int i = 0; i < snake.getJoints(); i++) {
        snake.setSnakeX(BOARDWIDTH / 2);
        snake.setSnakeY(BOARDHEIGHT / 2);
    }
    // Start off our snake moving right
    snake.setMovingRight(true);

    // Generate our first 'food'
    food.createFood();

    // set the timer to record our game's speed / make the game move
    timer = new Timer(speed, this);
    timer.start();
}

// if our snake is in the close proximity of the food..
void checkFoodCollisions() {

    if ((proximity(snake.getSnakeX(0), food.getFoodX(), 20))
            && (proximity(snake.getSnakeY(0), food.getFoodY(), 20))) {

        System.out.println("intersection");
        // Add a 'joint' to our snake
        snake.setJoints(snake.getJoints() + 1);
        // Create new food
        food.createFood();
    }
}

// Used to check collisions with snake's self and board edges
void checkCollisions() {

    // If the snake hits its' own joints..
    for (int i = snake.getJoints(); i > 0; i--) {

        // Snake cant intersect with itself if it's not larger than 5
        if ((i > 5)
                && (snake.getSnakeX(0) == snake.getSnakeX(i) && (snake
                        .getSnakeY(0) == snake.getSnakeY(i)))) {
            inGame = false; // then the game ends
        }
    }

    // If the snake intersects with the board edges..
    if (snake.getSnakeY(0) >= BOARDHEIGHT) {
        inGame = false;
    }

    if (snake.getSnakeY(0) < 0) {
        inGame = false;
    }

    if (snake.getSnakeX(0) >= BOARDWIDTH) {
        inGame = false;
    }

    if (snake.getSnakeX(0) < 0) {
        inGame = false;
    }

    // If the game has ended, then we can stop our timer
    if (!inGame) {
        timer.stop();
    }
}

void endGame(Graphics g) {

    // Create a message telling the player the game is over
    String message = "Game over";

    // Create a new font instance
    Font font = new Font("Times New Roman", Font.BOLD, 14);
    FontMetrics metrics = getFontMetrics(font);

    // Set the color of the text to red, and set the font
    g.setColor(Color.red);
    g.setFont(font);

    // Draw the message to the board
    g.drawString(message, (BOARDWIDTH - metrics.stringWidth(message)) / 2,
            BOARDHEIGHT / 2);

    System.out.println("Game Ended");

}

// Run constantly as long as we're in game.
@Override
public void actionPerformed(ActionEvent e) {
    if (inGame == true) {

        checkFoodCollisions();
        checkCollisions();
        snake.move();

        System.out.println(snake.getSnakeX(0) + " " + snake.getSnakeY(0)
                + " " + food.getFoodX() + ", " + food.getFoodY());
    }
    // Repaint or 'render' our screen
    repaint();
}

private class Keys extends KeyAdapter {

    @Override
    public void keyPressed(KeyEvent e) {

        int key = e.getKeyCode();

        if ((key == KeyEvent.VK_LEFT) && (!snake.isMovingRight())) {
            snake.setMovingLeft(true);
            snake.setMovingUp(false);
            snake.setMovingDown(false);
        }

        if ((key == KeyEvent.VK_RIGHT) && (!snake.isMovingLeft())) {
            snake.setMovingRight(true);
            snake.setMovingUp(false);
            snake.setMovingDown(false);
        }

        if ((key == KeyEvent.VK_UP) && (!snake.isMovingDown())) {
            snake.setMovingUp(true);
            snake.setMovingRight(false);
            snake.setMovingLeft(false);
        }

        if ((key == KeyEvent.VK_DOWN) && (!snake.isMovingUp())) {
            snake.setMovingDown(true);
            snake.setMovingRight(false);
            snake.setMovingLeft(false);
        }

        if ((key == KeyEvent.VK_ENTER) && (inGame == false)) {

            inGame = true;
            snake.setMovingDown(false);
            snake.setMovingRight(false);
            snake.setMovingLeft(false);
            snake.setMovingUp(false);

            initializeGame();
        }
    }
}

private boolean proximity(int a, int b, int closeness) {
    return Math.abs((long) a - b) <= closeness;
}

public static int getAllDots() {
    return TOTALPIXELS;
}

public static int getDotSize() {
    return PIXELSIZE;
}
}
\$\endgroup\$
  • \$\begingroup\$ I'm pretty new here but like the op, self taught over the course of many inconsistent sessions over many years. I came across this post by chance and loved every bit of it. I normally see forum posts like this riddled with super incomplete answers, random knowledge without direction and even sometimes just all bashing the developer (queue Reddit anyone?). At this time, I've made this account to come here and say that this was amazing. By far the best post I have read on the internet. Thank you for that. Looking forward to the day when I have something for you to review :) \$\endgroup\$ – DevClack Jan 23 '18 at 17:42
  • \$\begingroup\$ @DevClack Glad to hear that. I'd recommend taking a look at the tour. Also, once you have earned 20 reputation (which hopefully shouldn't take long) you'll be able to join us in chat \$\endgroup\$ – Simon Forsberg Jan 23 '18 at 19:41
8
\$\begingroup\$

Welcome to Code Review! There's lots that can be improved about this code, so lets get started.

Use the Random class instead of Math.random()

Instead of this

Math.random() * RANDOMPOSITION

Do this

int randomNumber = this.random.nextInt(RANDOMPOSITION)

The reason for this is that random.nextInt is more efficient and less biased (see here https://stackoverflow.com/questions/738629/math-random-versus-random-nextintint for more information about that).

Enums instead of booleans

Instead of having four booleans representing the direction that the snake is moving, instead make these into an enum. It could look like this:

public enum Direction {
    RIGHT, 
    LEFT, 
    UP, 
    DOWN;
}

Then, instead of this redundant code:

if ((key == KeyEvent.VK_LEFT) && (!snake.isMovingRight())) {
    snake.setMovingLeft(true);
    snake.setMovingUp(false);
    snake.setMovingDown(false);
}

You can just have snake.setDirection(Direction.LEFT)

I would also recommend replacing this one

// Check to see if the game is running
private boolean inGame = true;

With a GameState enum. That way, you can easily add a PAUSED and a GAME_OVER state depending on your needs.

Separate Game Model from Rendering

Looking at the Board class, it appears to be responsible for tying many different parts of the game together. I see the rendering, input, game state, game speed, game setup, game rules, and collision checking all in this one class.

It is good programming practice to separate the input and rendering from the underlying game model. This will allow you to switch out the rendering and input and still use the game model code almost unchanged. You can also port the game over to another language much more easily.

To achieve this, you will need a class that just handles the rendering and input. You could call this SnakeWindow. This class would be responsible for rendering the contents of the game board to the screen each frame, and for calling methods of the game model when receiving input. It would also update the game model each tick.

Object Oriented design

Following the idea of separating the game model from the rendering, there are many places where the code could be made more object oriented. It is not mandatory to write code in an object oriented way, but it is especially helpful when you are a beginner, and I personally find that it makes code easier to write and understand.

Below the SnakeWindow class there would be the SnakeGame class that would be responsible for the rules of the game. This class would have things like the speed of the game, or the speed that food spawns. It would have fields for the Snake object as well as the Board object. The window would update the SnakeGame each tick, and the game object would call the necessary methods of the snake and the board.

The Board object could have a simple int[][], and each position on the board could have a 0 for empty space, a 1 for a wall, and a 2 if a food is present. I think it would be best if it were responsible for the creation of food, so it could have a variable such as maxFood, and the Board would be called each tick to create the food.

Eventually you could change these integers to something more self documenting, such as an enum with values for FOOD, WALL, and EMPTY.

The window class would get information about the Snake and the Board from the game object and display this information on the screen. Related to this, I would remove all information about the dotSize from the game model and keep this information in the window. It's best to treat the board as discrete [x][y] locations and let the window be responsible for what size to draw them.

I would put the collision methods for the walls and food in the SnakeGame class, checking the positions of the snake against the positions of walls and food in the Board object. You could put the collision method for the snake against itself in this class, or alternatively put it inside the Snake class. For clarity though I would put it in the game class to keep all of the main game logic and rules in a central location.

The Food class

private final int RANDOMPOSITION = 40;
int location = (int) (Math.random() * RANDOMPOSITION);
foodX = ((location * Board.getDotSize()));

The fact that the max position of the food is 40 but then that number is multiplied by the dot size is confusing. It is also strange that the Food object has a field for the max position on the board at all. It means that this class is dependent on information that is actually contained in another class. I think it would be better to initialize the food object with values for the width and height of the board.

private Snake snake = new Snake();

Shouldn't there only be one instance of the Snake object? I'm not sure, but it appears that you are creating a snake object every time you create a food object. If you don't want food to spawn on spaces occupied by the snake, then I would pass a reference to the snake into the food object and allow it to check against those positions.

But really, I do not think that a Food class is even necessary. I think it would be better to let the Board be responsible for creating the food, and to do so it could just change an x and y position to the integer that represents food.

The Snake class

// Stores the joints / body part locations for our snake
private final int[] x = new int[Board.getAllDots()];
private final int[] y = new int[Board.getAllDots()];

I find it strange that the snake needs to have arrays that are the size of the entire board. Additionally, it seems to me that each joint of the snake should have a discrete x and y position. It doesn't make sense to me to keep track of these in two separate arrays.

I believe I would recommend using a Point or Joint class here to keep track of the x and y values. You could have an array or list of these objects, and when moving you could iterate over the list and move each point the way that it needs to be moved.

snake.setJoints(3); // set our snake's initial size

I think that the initial size of the snake should either be in the game object or inside the snake object.

// Create our snake's body
for (int i = 0; i < snake.getJoints(); i++) {
    snake.setSnakeX(BOARDWIDTH / 2);
    snake.setSnakeY(BOARDHEIGHT / 2);
}

Is it correct that all of the initial joints are being created on the same location? I think it would be better to initialize the Snake with a starting x and y position, and have the joints be created in the logical following positions in whatever direction you want.

I think that it is the wrong approach to treat the zero index of the array as the head of the snake. This relies on other areas of your code knowing that you must call the Snake methods with a 0 in order to get the position of the head. I think it would be better to have an explicit getSnakeHead method that would return the x and y position for the head for the purpose of collision detection.

Conclusion

I hope that I have not overwhelmed you with information. If you are going to continue doing game development in Java, I would highly recommend looking into libGDX. It has a much better structure for things such as the render loop, input handling, and screen management and transitions.

\$\endgroup\$
  • \$\begingroup\$ This is a very well-explained answer for a beginner. Nice job! ;) \$\endgroup\$ – KingCodeFish Jun 1 '16 at 23:40
  • \$\begingroup\$ Thanks for this! I most likely will not be implementing this for the current snake game, but rather for a future game I make. Just to make sure I am clear, should the overall structure be separated into many classes and let them all do their own thing? If I were to make a Pacman game, would I make a Board class that handles the creation of Pacman and the Ghosts? Then would I make a Pacman class which has its properties and a Ghost class which has its own properties? Would I make a renderer class which renders to the frame & handles key input? \$\endgroup\$ – tanu1215 Jun 3 '16 at 5:14
  • \$\begingroup\$ I can't really answer your questions about Pacman, but I think that you are generally correct. As far as separating things into classes, have a look at this en.wikipedia.org/wiki/Single_responsibility_principle and this en.wikipedia.org/wiki/Encapsulation_(computer_programming) \$\endgroup\$ – bazola Jun 3 '16 at 21:06
  • \$\begingroup\$ Yeah Pacman doesn't really matter. I was just asking about organization as a whole. It's my first time making games, so separating functionality into different classes is new to me (at least with games). Thanks again, means a lot. \$\endgroup\$ – tanu1215 Jun 3 '16 at 21:20
  • \$\begingroup\$ What do we do exactly inside the function setDirection(Direction.xxx) following your suggestion? \$\endgroup\$ – Kevin Randriajaoson Mar 8 '17 at 11:01

protected by Community Jan 23 '18 at 19:33

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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