I wrote a 2048 clone, looking for feedback

[Philipp Horn]

Hey,
I'm Philipp and I've been learning Java for the last few weeks, coming from a Python Background.
I just finished my first "bigger" project, a clone of the game 2048. There are a few things left to add, but I am mostly finished with it.
I am still a bit confused by access modifiers, mostly I just followed the rule of thumb to keep everything as private as possible. Is it a bad habbit to change my methods from private to default for easier testing?
It would be awesome if you could give me constructive criticism for my code, I'm open to anything

https://github.com/mogl0768/2048-java

[Winston Gutkowski]

Philipp Horn wrote:I am still a bit confused by access modifiers, mostly I just followed the rule of thumb to keep everything as private as possible. Is it a bad habbit to change my methods from private to default for easier testing?

Yes.

It would be awesome if you could give me constructive criticism for my code, I'm open to anything

Well, a few things I see, from your Game class (I'm not qualified to advise on GUI code):
1. Why is direction an int[]? My assumption is that it would be some sort of value like "UP"/"DOWN"/"LEFT"/"RIGHT".
2. Don't use Math.random(). Use Random.nextInt() instead.
3. You don't have to fill an int array with 0's because that's the default.
4. Your program looks very "procedural" to me. In particular, I suspect that you could cut down considerably on the number of parameters you pass around by adding a Grid class that encapsulates your grid and all its actions, leaving your Game class simply as a "manager" or "translator" of user moves and arbiter of when a Player has won or lost.

Also: We generally prefer that you post your code here, as some people don't like to go to sites like github; so you will get more responses if you do.
And before you do, please read the UseCodeTags page.

Oh, and welcome to JavaRanch, Philipp.

Winston

[Philipp Horn]

Winston Gutkowski wrote:
1. Why is direction an int[]? My assumption is that it would be some sort of value like "UP"/"DOWN"/"LEFT"/"RIGHT".

I chose to represent the direction by a vector in the form of {x, y}. So {-1, 0} would be left for example. I thought about using a string or constant, but since I am using the vector directly in the functions and it was still pretty understandable to me, I went with that.

Thanks a lot for your feedback, especially the part about my program being procedural kind of opened my eyes. Towards the end of the program I realized myself that my code got pretty hard to extend.
I'll post my GUI code here, in case anyone has a few points to improve on it:

public class GUI extends JFrame implements KeyListener{
	

	private Game gameob = new Game();
	private JFrame frame = new JFrame("2048");
	private JPanel panel = new JPanel();
	private HashMap<Integer, int[]> keyToDirection = new HashMap<Integer, int[]>();
	
	private GUI(){
		setSize(550, 550);
		setTitle("2048");
		setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
		add(panel);
		panel.setLayout(new GridLayout(0, 4));
		addKeyListener(this);
		// Map keys and directions:
		keyToDirection.put(37, gameob.LEFT);
		keyToDirection.put(38, gameob.UP);
		keyToDirection.put(39, gameob.RIGHT);
		keyToDirection.put(40, gameob.DOWN);
	}
	public static void main(String[] args){
		

		GUI gui = new GUI();
		gui.startGame();
		gui.drawGrid();
		gui.setVisible(true);
	}
	
	private void startGame(){
		gameob.spawnBlock();
		gameob.spawnBlock();
	}
	
	private void drawGrid(){
		
		for (int[] line : gameob.getGrid()){
			for (int column : line){
				// Only show number if it is not zero
				String displayedNumber = column != 0 ? String.valueOf(column) : "";
		
				JLabel label = new JLabel(displayedNumber, SwingConstants.CENTER);
				label.setBorder(BorderFactory.createLineBorder(Color.BLACK));
				
				int fontSize = getFontsize(column);
				label.setFont(new Font("TimesRoman", Font.PLAIN, fontSize));
				panel.add(label);
			}
		}
	}

	private int getFontsize(int number){
		// finds fitting font size to display the number
		if (number < 100){
			return 125;
		} if (number < 1000){
			return 80;
		} else {
			return 50;
		}
	}

	public void keyPressed(KeyEvent e) {
		int key = e.getKeyCode();
		if (keyToDirection.containsKey(key) &&
				gameob.move(keyToDirection.get(key))){
			
			gameob.spawnBlock();
			update();
		}
		if (gameob.isLost()){
			System.out.println("You lost!");
			gameob = new Game();
			startGame();
			update();
		}
		
	}
	private void update(){
		panel.removeAll();
		drawGrid();
		invalidate();
		validate();
		repaint();
	}

	public void keyReleased(KeyEvent arg0){}

	public void keyTyped(KeyEvent arg0){}

}

[Stephan van Hulst]

Don't let GUIs implement listeners. This is a bad technique that sadly is taught in way too many tutorials and books. Use a nested class, or even better, instantiate your listeners with a lambda expression. The reason is that by implementing KeyListener, you're essentially saying that your GUI can be used as an all purpose keystroke handler, even in other classes that it has nothing to do with. You should also try to avoid extending JFrame. Your GUI uses a frame, it isn't necessarily a frame itself. I see you already have a frame member field which is unused right now. Use it.

Your frame, panel and keyToDirection members can be final. keyToDirection should be a Map, not a HashMap. If the mappings don't change between GUI instances, you can make the keyToDirection static, and initialize its entries in a static intializer block.

I don't think you should be using an int[] to represent a direction. Create a Direction enum instead.

Don't use magic values in your key mapping. Use the constants defined in KeyEvent instead.

Interaction with Swing components should be done on the Event Dispatch Thread:

SwingUtilities.invokeLater(() -> {
  GUI gui = new GUI();
  // etc.
});

Try to avoid creating new components after the GUI has been made visible. Instantiate your labels when you create the GUI, and just change their text value during the course of a game. You can then get rid of the call to validate(). Even if you don't go this route, you should call revalidate() instead of validate().

If you post more of your code instead of linking to it, we can offer some more advice on that as well.

[Philipp Horn]

Thanks, that helped a lot. I didn't look into threads yet, but I think i understood the rest of your points and I already implemented the enum for directions (and it turned out to be pretty useful ).
These are my updated Grid and Game classes, this time it should be more object oriented.
Direction enum:

public enum Direction {
	LEFT (-1, 0),
	RIGHT (1,0),
	UP (0, -1),
	DOWN(0, 1);
	
	public final int x;
	public final int y;
	
	Direction(int x, int y){
		this.x = x;
		this.y = y;
	}
	public Direction oppositeDirection(){
		if (this == Direction.UP) {return Direction.DOWN;}
		if (this == Direction.DOWN) {return Direction.UP;}
		if (this == Direction.RIGHT) {return Direction.LEFT;}
		else {return Direction.RIGHT;}
	}
	public Direction otherDimension(){
		//used to get start points for merging
		return this.x == 0 ? RIGHT : DOWN;
	}
}

Grid class:

public class Grid {
	private int[][] grid;
	private int size;
	
	public Grid(int inputSize){
		size = inputSize;
		grid = new int[size][size];	
	}
	
	public void setgrid(int[][]grid){
		this.grid = grid;
	}
	
	public int getValue(int x, int y){
		return grid[y][x];
	}
	
	public void setValue(int x, int y, int value){
		grid[y][x] = value;
	}
	
	public int getSize(){
		return size;
	}
	
	public Grid clone(){
		int [][] newgrid = new int[size][size];
		for (int i = 0; i < grid.length; i++){
			newgrid[i] = Arrays.copyOf(grid[i], grid[i].length);
		}
		Grid copyGrid = new Grid(size);
		copyGrid.grid = newgrid;
		return copyGrid;
	}
	
	public int[] getLine(int[] point, Direction direction){
		
		int[] output = new int[size];
		int i = 0;
		for (int[] pos : genPositions(point, direction)){
			output[i] = grid[pos[1]][pos[0]];
			i++;
		}
		return output;
	}
	
	public int[][] getGrid(){
		return grid;
	}
	
	public void setLine(int[] point, Direction direction, int[] line){
		int linePos = 0;
		for (int[] pos : genPositions(point, direction)){
			grid[pos[1]][pos[0]] = line[linePos];
			linePos++;
		}
	}
	
	public boolean isFull(){
		//checks if grid is full
		for(int[] row : grid){
			for (int value : row){
				if (value == 0){
					return false;
				}
			}
		}
		return true;
	}
	
	public int[][] genPositions(int[] point, Direction direction){
		/*generate all positions passed, going from the given point in the given direction
		 * returns array of positions
		 */
		int[][] output = new int[size][2];
		int x = point[0];
		int y = point[1];
		for (int i = 0; i < size; i++){
			output[i][0] = x;
			output[i][1] = y;
			x += direction.x;
			y += direction.y;
		}
		return output;
	}
	
	public boolean equals( Object obj ){
		if (!(obj instanceof Grid)){
			return false;
		}
		Grid otherGrid = (Grid) obj;
		return Arrays.deepEquals(grid, otherGrid.getGrid());
	}
	
	public int hashCode(){
		return size;
	}
}

Game class:

public class Game {
	
	final int SIZE = 4;
	Grid gameGrid = new Grid(SIZE);
	Random random = new Random();
	
	public boolean move(Direction direction){
		// move given grid in given direction, returns if the Grid changed
		Grid gridCopy = gameGrid.clone();
		for (int [] point : genStartPoints(direction)){
			int[] line = gameGrid.getLine(point, direction.oppositeDirection());
			gameGrid.setLine(point, direction.oppositeDirection(), merge(line));
		}
		return !gameGrid.equals(gridCopy);
	}
	
	private int[][] genStartPoints(Direction direction){
		// generate all start points to generate the merged lines 

		int[] startPoint = new int[2];
		int gridEnd = gameGrid.getSize() - 1;
		startPoint[0] = direction.x == 1 ? gridEnd : 0;
		startPoint[1] = direction.y == 1 ? gridEnd : 0;
		return gameGrid.genPositions(startPoint, direction.otherDimension());
		
	}
	
	private int[] merge(int[] line) {
		// merges the given array to the left side
		int valueBuffer = 0;
		int[] output = {0, 0, 0, 0};
		int outputInd = 0;
		for (int number : line) {
			if (number != 0) {
				if (number == valueBuffer) {
					output[outputInd] = number + valueBuffer;
					outputInd++;
					valueBuffer = 0;
				} else if (valueBuffer != 0) {
					output[outputInd] = valueBuffer;
					outputInd++;
					valueBuffer = number;
				} else {
					valueBuffer = number;
				}
			}
		}
		if (valueBuffer != 0) {output[outputInd] = valueBuffer;}
		return output;
	}
	
	public void spawnBlock(){
		// spawns new number at random location
		int rand1;
		int rand2;
		if (!gameGrid.isFull()){
			while(true){
				rand1 = random.nextInt(SIZE);
				rand2 = random.nextInt(SIZE);
				if (gameGrid.getValue(rand1, rand2) == 0){
					// there is a 75% percent chance for a two:
					int number = random.nextInt(5) != 4 ? 2 : 4;
					gameGrid.setValue(rand1, rand2, number);
					break;
				}
			}
		}
	}
	public boolean isLost(){
		// checks if the game is lost
		// make new game object with same grid:
		Game gameCopy = new Game();
		gameCopy.setGrid(gameGrid.clone());
		// move in each direction, if the grid changes, the game is not lost
		for (Direction direction : Direction.values()){
			if (gameCopy.move(direction)) {
				return false;
			}
		}
		return true;
	}
	private void setGrid(Grid grid){
		this.gameGrid = grid;
	}
	public int getGridValue(int x, int y){
		// used by the GUI to display the game
		return gameGrid.getValue(x, y);
	}
}

[Paul Clapham]

An alternate way for assigning attributes to Enum elements (unrelated code omitted in this example):

public enum Direction {
    LEFT (-1, 0) {
      public Direction oppositeDirection() {
        return RIGHT;
      }
    },
    RIGHT (1,0) {
      public Direction oppositeDirection() {
        return LEFT;
      }
    },
    UP (0, -1) {
      public Direction oppositeDirection() {
        return DOWN;
      }
    },
    DOWN(0, 1) {
      public Direction oppositeDirection() {
        return UP;
      }
    };

    public abstract Direction oppositeDirection();

}

You may find this more or less convenient; it's more self-documenting but on the other hand there's a bit more code necessary to support it.