2048 game in Java

Question

I am a beginner learning Java, and I coded a command line version of the game 2048 for practice. Any feedback, especially regarding best practices, object-oriented principles, and tidying up the code logic, is appreciated.

import java.util.Scanner; class Cell { int value = 0; boolean hasMerged = false; } class Game { final int BOARD_SIZE = 4; Cell[][] cells; void create() { cells = new Cell[BOARD_SIZE][BOARD_SIZE]; for (int i = 0; i < BOARD_SIZE; i++) { for (int j = 0; j < BOARD_SIZE; j++) { cells[i][j] = new Cell(); cells[i][j].value = 0; } } } int rowStart = 0; int columnStart = 0; int rowStep = 0; int columnStep = 0; int nextRow = 0; int nextColumn = 0; //to be used for move() and isLegal() void setDirection (char direction) { switch (direction) { case 'W': rowStart = 1; columnStart = 0; rowStep = 1; columnStep = 1; nextRow = -1; nextColumn = 0; break; case 'A': rowStart = 0; columnStart = 1; rowStep = 1; columnStep = 1; nextRow = 0; nextColumn = -1; break; case 'S': rowStart = 2; columnStart = 0; rowStep = -1; columnStep = 1; nextRow = 1; nextColumn = 0; break; case 'D': rowStart = 0; columnStart = 2; rowStep = 1; columnStep = -1; nextRow = 0; nextColumn = 1; break; } } void move() { boolean changed = true; while (changed) { changed = false; for (int i = rowStart; i >= 0 && i < BOARD_SIZE; i += rowStep) { for (int j = columnStart; j >= 0 && j < BOARD_SIZE; j += columnStep) { if (cells[i][j].value != 0 && cells[i + nextRow][j + nextColumn].value == 0) { changed = true; cells[i + nextRow][j + nextColumn].value = cells[i][j].value; cells[i + nextRow][j + nextColumn].hasMerged = cells[i][j].hasMerged; cells[i][j].value = 0; cells[i][j].hasMerged = false; } else if (cells[i][j].value != 0 && cells[i + nextRow][j + nextColumn].value == cells[i][j].value && !(cells[i][j].hasMerged || cells[i + nextRow][j + nextColumn].hasMerged)) { changed = true; cells[i + nextRow][j + nextColumn].value *= 2; cells[i + nextRow][j + nextColumn].hasMerged = true; cells[i][j].value = 0; cells[i][j].hasMerged = false; } } } } for (int i = 0; i < BOARD_SIZE; i++) { for (int j = 0; j < BOARD_SIZE; j++) { cells[i][j].hasMerged = false; } } } void generateRandomCell() { int count = 0; int[][] empty = new int[16][2]; for (int i = 0; i < BOARD_SIZE; i++) { for (int j = 0; j < BOARD_SIZE; j++) { if (cells[i][j].value == 0) { empty[count][0] = i; empty[count][1] = j; count += 1; } } } int randomIndex = (int) (Math.random() * count); if (Math.random() < 0.9) { cells[empty[randomIndex][0]][empty[randomIndex][1]].value = 2; } else { cells[empty[randomIndex][0]][empty[randomIndex][1]].value = 4; } } boolean isLegal() { for (int i = rowStart; i >= 0 && i < BOARD_SIZE; i += rowStep) { for (int j = columnStart; j >= 0 && j < BOARD_SIZE; j += columnStep) { if (cells[i][j].value != 0 && (cells[i + nextRow][j + nextColumn].value == 0 || cells[i][j].value == cells[i + nextRow][j + nextColumn].value)) { return true; } } } return false; } boolean isOver() { char[] directions = {'W', 'A', 'S', 'D'}; for (int i = 0; i < BOARD_SIZE; i++) { setDirection(directions[i]); if (isLegal()) { return false; } } return true; } void printBoard() { int width = 5; int space; System.out.print("\n"); for (int i = 0; i < BOARD_SIZE; i++) { for (int j = 0; j < BOARD_SIZE; j++) { space = width - String.valueOf(cells[i][j].value).length(); if (cells[i][j].value != 0) { System.out.print(cells[i][j].value); } else { System.out.print("_"); } System.out.print(" ".repeat(space)); } System.out.print("\n\n"); } System.out.println(""); } } class Game2048 { public static void main(String[] args) { Scanner scanner = new Scanner(System.in); Game game = new Game(); game.create(); game.generateRandomCell(); System.out.print("\n\nHello!\nEnter W, A, S, D to move, and Q to quit.\n\n"); while (!game.isOver()) { game.printBoard(); System.out.println("Enter your move: "); String userInput = scanner.nextLine(); if (userInput.length() != 1) { System.out.print("\nInvalid!\n"); continue; } char userMove = userInput.charAt(0); if (userMove == 'Q') { System.exit(0); } else if (userMove != 'W' && userMove != 'A' && userMove != 'S' && userMove != 'D') { System.out.print("\nInvalid!\n"); continue; } game.setDirection(userMove); if (game.isLegal()) { game.move(); game.generateRandomCell(); } else { System.out.print("\nIllegal!\n"); } } System.out.println("Game over!"); } } 

Answer 1

UX

• it would be more convenient if both lower and upper case letters were accepted. This can be achieved easily using Character.toUpperCase(...) method when initializing userMove in main.
• it's quite annoying to have to press Enter each time, since every input is always just 1 char anyway. I've just learnt, that there's no portable way to fix it without an additional lib. For mac & linux you can follow this 2 line trick and then use java.io.Reader obtained with System.console().reader(), but if you want it to work on M$ Windows also, then you need to use for example jLine or jCurses.
• it would be even better if arrow keys were also accepted (in addition to w, a, s, d).

Organizing code dealing with UI

Whenever you are developing an interactive program that has some user-interface (regardless if it's a text-terminal UI, or some graphic UI like desktop Swing, mobile Android or web GWT), it is a good habit to clearly divide your code into the following parts:

• a part that deals solely with core logic of a given issue, usually called a Model or an Engine, in case of "2048" game that would be a class holding the state of the board with methods to obtain such state and to make a move.
• a part that deals solely with a given UI, usually divided into Screens or Views or Submenus, in case of "2048" game, there's only 1 such View displaying the board and capturing user input on it.
• finally some code that glues the previous 2 parts together, depending on its design called a Presenter or a Controller or a ViewModel or other...

Model and View parts usually have abstract interfaces for the gluing part to deal with them, so in case of "2048" game they could be looking for example something like this:

public interface Game2048Model { GameState performAction(Action action) throws IllegalMoveException; GameState getGameState(); // for UI redrawing and to see the initial board class GameState { public State state; public int[][] board; } enum State {LOST, WON, ONGOING} enum Action {LEFT, RIGHT, UP, DOWN, RESTART} class IllegalMoveException extends Exception {} } public interface Game2048View { void updateBoardDisplay(int[][] newBoardState); void updateStateDisplay(Game2048Model.State newState); void signalIllegalMove(); void registerUserInputListener(Consumer<UserInput> listener); enum UserInput {LEFT, RIGHT, UP, DOWN, RESTART, QUIT} // Action + QUIT // registerUserInputListener(listener) is the way it is, because most // GUI frameworks are event-driven. If it's confusing at this stage, // in case of text-UI only, you can replace it with // UserInput getUserInput(); } 

Among several others, such approach has the following benefits:

• for anyone looking at these interfaces, it will be immediately clear what a user can do during the game and roughly how the UI may look like (regardless again if it's a text or some GUI).
• abstract UserInput enum forces you to put details of input validation/recognition into the UI layer (checking characters from keyboard for text-UI, checking which button was tapped for mobile and so on), making the higher-level glue code easier to read and independent from the UI type.
• if you decide to provide another type of UI apart from the current text one, you can reuse your Model (and possibly the glue part if designed appropriately) and just provide a new implementation of Game2048View (using Swing/Android/GWT widgets). Also any changes in layout that you may want to do later, will not affect the other 2 parts.
• you can easily write tests that concern only the Model right away, you can write tests that concern only the gluing part by providing mock implementations for both Game2048Model and Game2048View (for example using EasyMock or Mockito).

There are a few "well-established" design patterns related to this, 2 of the simplest and most popular are model-view-presenter and model-view-controller, which in case of an app with a single screen will result in roughly similar code.

Game class

create()

This code should be moved to constructor basically.

generateRandomCell()

• use BOARD_SIZE*BOARD_SIZE instead of 16.
• using the same random value both for choosing the cell and deciding whether put there 2 or 4 determines which cells may get 2 and which 4 when chosen.
• instead of (int) (Math.random() * count) it's probably better to do have an instance of Random and do random.nextInt(count).

isOver()

• It seems that the current implementation does not allow to win :-( Even if a user reaches 2048 the game continues until there are no legal moves and then the user will get "Game over!". I feel cheated ;-)
• for loop iterates over directions array using BOARD_SIZE as the limit, which coincidently happens to be both 4. If you wanted to try to play the game on a bigger board it would crash. Use directions.length instead or even better for (var direction: directions).

printBoard()

For simple text formatting use String.format(...) or System.out.printf(...): no need to count spaces manually ;-)

setDirection(userMove) and paramless isLegal() & move()

This is really ugly and misleading. I started reading main code first and it really made me scratch my head for few minutes wondering how isLegal() and move() work without providing any actual move data :? Only after a while I noticed that it so happens that setDirection(userMove) is always called upfront sets a "temporary state" for them... setDirection(userMove) should not exist (or at least not be accessible) and both isLegal() and move() should have a userMove param.
As a first fix, setDirection(userMove) may be made private and called at the beginning of isLegal(userMove) and move(userMove). It's still ugly though, as it uses a "temporary" instance state instead of local vars. To fix this, you can introduce a simple static inner class called for example DirectionConfig with these 6 fields (rowStart, columnStart, rowStep, columnStep, nextRow, nextColumn) and change setDirection(userMove) to return a new instance of this class, that move(userMove) and isLegal(userMove) would use instead of Game's instance fields (in such case you can also rename setDirection(userMove) to getDirectionConfig(userMove)).

rowStart, columnStart, rowStep, columnStep, nextRow, nextColumn usage in move() and isLegal()

These are SIX input parameters for a pretty complex board matrix processing using TWO additional vars (changed and Cell.hasMarged), but OTOH there's ZERO javadoc, comment or any other explanation... I would prefer to carry concrete bricks at some construction yard for half a day, than to be forced to understand this code with this level of documentation ;-)

Cell class

• it is just a tiny helper that will never be used outside of Game, so it should rather be an inner class (static).
• it serves both as a holder of the board state and as a holder of a temporary flag hasMerged used in Game.move(). Combining these 2 purposes is really ugly and confusing. At the very least, the meaning of this flag should be documented.

Game2048 class

Currently in only has main method that could simply be moved to Game

Answer 2

Overview

Generally speaking, the code layout looks good and you used meaningful names for variables, functions and classes.

That being said, it would be better to reduce the length of the 2 longest lines of code.

Portability

I got an error on the following line when I tried to compile the code:

 System.out.print(" ".repeat(space)); 

Perhaps I am running on a different version of Java. As a hack to get it to compile, I removed .repeat(space).

Instructions

When I ran the code, I ended up getting the message:

 Illegal! 

It is good that the code reports an error, but it would be better if it also reported why it is an error and what I should do to correct my actions.

Along the same lines, it would be better to post a few more lines of instructions when the code is run, such as the name of the game, the objective of the game, etc.

Input

It is great that you have code that checks user input. Consider allowing lowercase letters for the input: w in addition to W, d in addition to D, etc. Your code could convert all input to uppercase. That is pretty common for user input.

Tabs

When I copied the code from your question into an editor, some of the indentation was inconsistent. I suspect (but I can not easily prove) that some lines of your code have both tab characters and single spaces. If that is the case, consider using just one or the other.