patternjavaMinor
First game of Snake
Viewed 0 times
gamefirstsnake
Problem
I'm making baby steps in Java programming and I just ended coding basic game: Snake. Could you share with me what is needed to improve in this code? What about OOP here?
GameBoard.java
```
import javax.swing.*;
import java.awt.*;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.util.LinkedList;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
public class GameBoard extends JFrame {
//setting board size
public static int boardWidth = 1000;
public static int boardHeight = 800;
private JPanel scorePanel;
public static void main(String[] args) {
new GameBoard();
}
public GameBoard() {
super("Snake Game");
this.setSize(boardWidth, boardHeight);
this.setResizable(false);
this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
addKeyListener(new KeyListener() {
@Override
public void keyTyped(KeyEvent e) {
}
@Override
public void keyPressed(KeyEvent e) {
if (e.getKeyCode() == KeyEvent.VK_LEFT) {
DrawingTheBoard.theSnake.changeDirection(Direction.LEFT);
} else if (e.getKeyCode() == KeyEvent.VK_UP) {
DrawingTheBoard.theSnake.changeDirection(Direction.UP);
} else if (e.getKeyCode() == KeyEvent.VK_RIGHT) {
DrawingTheBoard.theSnake.changeDirection(Direction.RIGHT);
} else if (e.getKeyCode() == KeyEvent.VK_DOWN) {
DrawingTheBoard.theSnake.changeDirection(Direction.DOWN);
}
}
@Override
public void keyReleased(KeyEvent e) {
}
});
DrawingTheBoard gamePanel = new DrawingTheBoard();
this.add(gamePanel, BorderLayout.CENTER);
scorePanel = new JPanel();
scorePanel.add(gamePanel.scoreLabel, BorderLayout.CENTER);
this.add
GameBoard.java
```
import javax.swing.*;
import java.awt.*;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.util.LinkedList;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
public class GameBoard extends JFrame {
//setting board size
public static int boardWidth = 1000;
public static int boardHeight = 800;
private JPanel scorePanel;
public static void main(String[] args) {
new GameBoard();
}
public GameBoard() {
super("Snake Game");
this.setSize(boardWidth, boardHeight);
this.setResizable(false);
this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
addKeyListener(new KeyListener() {
@Override
public void keyTyped(KeyEvent e) {
}
@Override
public void keyPressed(KeyEvent e) {
if (e.getKeyCode() == KeyEvent.VK_LEFT) {
DrawingTheBoard.theSnake.changeDirection(Direction.LEFT);
} else if (e.getKeyCode() == KeyEvent.VK_UP) {
DrawingTheBoard.theSnake.changeDirection(Direction.UP);
} else if (e.getKeyCode() == KeyEvent.VK_RIGHT) {
DrawingTheBoard.theSnake.changeDirection(Direction.RIGHT);
} else if (e.getKeyCode() == KeyEvent.VK_DOWN) {
DrawingTheBoard.theSnake.changeDirection(Direction.DOWN);
}
}
@Override
public void keyReleased(KeyEvent e) {
}
});
DrawingTheBoard gamePanel = new DrawingTheBoard();
this.add(gamePanel, BorderLayout.CENTER);
scorePanel = new JPanel();
scorePanel.add(gamePanel.scoreLabel, BorderLayout.CENTER);
this.add
Solution
General
You are quite fine with the separation of concerns. You identified the most important parts of the game and you gave them meaningful names.
As your approach is semantical I'd like to follow it and make some hints in which semantical direction the code will be even more plausible.
Finally I will make some more technical suggestions.
Game instance
Currently your Gameboard is responsible to start over again. You have to reset the snake, reset the score. What you are doing here is: You recycle a "game instance". In this simple case this is not that a problem. If you have complex objects you may forget reinitializing some of the values. This is error prone if you do not make this to a "big concept" (like a state pattern) OR your introduce "Game Instances" that you can dismiss if the "game is over".
Direction management
I think you did not handle the aspect "One direction change per tick". If someone enters the direction very fast there can be the problem that the snake is turning 180 degrees immediately.
To avoid this you have to change the semantic "changing the direction of a snake" to "make a direction change request". You should store the requested direction and evaluate it everytime your game makes a tick.
One other thing is, that your direction enum could be used to let polymorphism work to simplifiy your code.
Your update-method could look like this:
Apple
To get the semantic right: An apple does not place itself on the game board. It is placed by ... yeah ... that has to be identified. Maybe it is a game rule that has to be applied at the very beginning of a game and after the snake has eaten the apple. An apple can be eaten and placed only if the game makes a tick.
This is a game rule that will be applied to a concrete game instance at a tick timepoint.
At the moment the apple is nothing more than an empty shell to create a Point-Object. The semantic of an apple forgotton as soon as it was instantiated that is sad as you could pass it into methods to make things more explicit rather than passing around "abstract points".
Gameboard
You have encrypted the game board fields in UI calculation. My prefered way is to have such essential information stated clearly in the code like "the game board has 10x8 fields". Any other information should be derived from this.
Maybe you should introduce a 2D array of Gameboardelements as most gameboards have such a structure. Think about a chess board, connect four or Tic Tac Toe. The highest similarity to the reality is a 2D array to represent a game board.
Snake
The addPartOfBody-method can be reduced in visibility. Use the "private" modifier if you can as you show too much inner state to other objects. The goal should be to keep your public interface as small as possible.
The aspect "eating the apple" is really hidden and handled deep inside the snake. ;-). On the one hand I am kidding on the other hand I am serious because the only code artefact that reminds someone is the name of the variable "applePoint". I suggest to at least pass the apple. But that would not be my favorite way.
I would detect the apple collision outside the snake and let the snake eat the apple. If an apple was passed then the snake will grow. Something like this:
The eat-method:
This is only an idea. Keep the semantics right and your code will be more clear.
The trick is to identify some really clear statements and structures and develop the other aspects strictly around them.
Coupling
You have a strong coupling with the UI. My way to have as less coupling as possible is to start implementing the game without any UI. And if I say UI I also mean any visual representation of the game like console output/input.
If you restrict yourself to this methodological process of implementation you get at least some essential decoupling out of the box.
JFrame exit on close
I suggest to use DISPOSE_ON_CLOSE rather that using EXIT_ON_CLOSE. This forces you to think about proper shutdown mechanisms for your application. Using EXIT_ON_CLOSE is like "I don't care about application state". DISPOSE_ON_CLOSE preserves you the knowledge about all resources being used in your application and handle their shutdown properly.
Avoid multiple return statements
Even if the method is really small (and that's fine) you should consider to avoid multiple return statements. Return statements hinder you to apply
You are quite fine with the separation of concerns. You identified the most important parts of the game and you gave them meaningful names.
As your approach is semantical I'd like to follow it and make some hints in which semantical direction the code will be even more plausible.
Finally I will make some more technical suggestions.
Game instance
Currently your Gameboard is responsible to start over again. You have to reset the snake, reset the score. What you are doing here is: You recycle a "game instance". In this simple case this is not that a problem. If you have complex objects you may forget reinitializing some of the values. This is error prone if you do not make this to a "big concept" (like a state pattern) OR your introduce "Game Instances" that you can dismiss if the "game is over".
Direction management
I think you did not handle the aspect "One direction change per tick". If someone enters the direction very fast there can be the problem that the snake is turning 180 degrees immediately.
To avoid this you have to change the semantic "changing the direction of a snake" to "make a direction change request". You should store the requested direction and evaluate it everytime your game makes a tick.
One other thing is, that your direction enum could be used to let polymorphism work to simplifiy your code.
...
LEFT {
Direction opposite() {
return RIGHT;
}
@Override
int getX() {
return -1;
}
@Override
int getY() {
return 0;
}
}
...Your update-method could look like this:
...
public void update(Point applePoint) {
addPartOfBody(headDirection.getX() * 10, headDirection.getY() * 10, applePoint);
}
...Apple
To get the semantic right: An apple does not place itself on the game board. It is placed by ... yeah ... that has to be identified. Maybe it is a game rule that has to be applied at the very beginning of a game and after the snake has eaten the apple. An apple can be eaten and placed only if the game makes a tick.
This is a game rule that will be applied to a concrete game instance at a tick timepoint.
At the moment the apple is nothing more than an empty shell to create a Point-Object. The semantic of an apple forgotton as soon as it was instantiated that is sad as you could pass it into methods to make things more explicit rather than passing around "abstract points".
Gameboard
You have encrypted the game board fields in UI calculation. My prefered way is to have such essential information stated clearly in the code like "the game board has 10x8 fields". Any other information should be derived from this.
Maybe you should introduce a 2D array of Gameboardelements as most gameboards have such a structure. Think about a chess board, connect four or Tic Tac Toe. The highest similarity to the reality is a 2D array to represent a game board.
Snake
The addPartOfBody-method can be reduced in visibility. Use the "private" modifier if you can as you show too much inner state to other objects. The goal should be to keep your public interface as small as possible.
The aspect "eating the apple" is really hidden and handled deep inside the snake. ;-). On the one hand I am kidding on the other hand I am serious because the only code artefact that reminds someone is the name of the variable "applePoint". I suggest to at least pass the apple. But that would not be my favorite way.
I would detect the apple collision outside the snake and let the snake eat the apple. If an apple was passed then the snake will grow. Something like this:
if (collides(snake, apple)) {
Apple apple = takeAppleFromGameBoard();
snake.eat(apple);
}The eat-method:
public class Snake {
....
public void eat(Apple apple) {
if (apple != null) {
grow();
}
}
....
}This is only an idea. Keep the semantics right and your code will be more clear.
The trick is to identify some really clear statements and structures and develop the other aspects strictly around them.
Coupling
You have a strong coupling with the UI. My way to have as less coupling as possible is to start implementing the game without any UI. And if I say UI I also mean any visual representation of the game like console output/input.
If you restrict yourself to this methodological process of implementation you get at least some essential decoupling out of the box.
JFrame exit on close
I suggest to use DISPOSE_ON_CLOSE rather that using EXIT_ON_CLOSE. This forces you to think about proper shutdown mechanisms for your application. Using EXIT_ON_CLOSE is like "I don't care about application state". DISPOSE_ON_CLOSE preserves you the knowledge about all resources being used in your application and handle their shutdown properly.
Avoid multiple return statements
Even if the method is really small (and that's fine) you should consider to avoid multiple return statements. Return statements hinder you to apply
Code Snippets
...
LEFT {
Direction opposite() {
return RIGHT;
}
@Override
int getX() {
return -1;
}
@Override
int getY() {
return 0;
}
}
......
public void update(Point applePoint) {
addPartOfBody(headDirection.getX() * 10, headDirection.getY() * 10, applePoint);
}
...if (collides(snake, apple)) {
Apple apple = takeAppleFromGameBoard();
snake.eat(apple);
}public class Snake {
....
public void eat(Apple apple) {
if (apple != null) {
grow();
}
}
....
}Context
StackExchange Code Review Q#146972, answer score: 4
Revisions (0)
No revisions yet.