2048 clone in libgdx, handling assets, and OOP overkill?

Question

I have created a 2048 clone, I wanted to take on a simple project since I have never actually finished anything. Now I have, though I'm not a hundred percent happy with the result.

Images so you know what we're discussing:

My concerns

• Handling assets

This is the first project in which I use the AssetManager class. I have handled assets by giving the assetManager to each object in its constructor, is this how I'm supposed to do this? Also, I use a few different font sizes, so I had to do this:

FileHandleResolver resolver = new InternalFileHandleResolver();
    assetManager.setLoader(FreeTypeFontGenerator.class, new FreeTypeFontGeneratorLoader(resolver));
    assetManager.setLoader(BitmapFont.class, ".ttf", new FreetypeFontLoader(resolver));

    FreetypeFontLoader.FreeTypeFontLoaderParameter font = new FreetypeFontLoader.FreeTypeFontLoaderParameter();
    font.fontFileName = "ClearSans-Bold.ttf";
    font.fontParameters.size = HEIGHT / 10;
    assetManager.load("mediumFont.ttf", BitmapFont.class, font);

    FreetypeFontLoader.FreeTypeFontLoaderParameter font2 = new FreetypeFontLoader.FreeTypeFontLoaderParameter();
    font2.fontFileName = "ClearSans-Bold.ttf";
    font2.fontParameters.size = HEIGHT / 6;
    assetManager.load("bigFont.ttf", BitmapFont.class, font2);

    FreetypeFontLoader.FreeTypeFontLoaderParameter font3 = new FreetypeFontLoader.FreeTypeFontLoaderParameter();
    font3.fontFileName = "ClearSans-Regular.ttf";
    font3.fontParameters.size = HEIGHT / 30;
    assetManager.load("smallFont.ttf", BitmapFont.class, font3);

etc.

Is there a better way to handle many different font sizes? I already do this for the size of the numbers on the tiles:

void setFontSize() {
    BitmapFont tileFont = assetManager.get("tileFont.ttf", BitmapFont.class);
    tileFont.getData().setScale(2.0f / BOARD_SIZE);

    BitmapFont smallTileFont = assetManager.get("smallTileFont.ttf", BitmapFont.class);
    smallTileFont.getData().setScale(2.0f / BOARD_SIZE);
}

• Handling constants

I have a few constants like the color of certain text elements, should I create a new class and create color objects in them?

• OOP question

I have created new classes for a loss screen and win screen. They of course both only need to be instanced once, and they always look the same, should I not have used OOP for this? Here is the loss class and how I show it:

LossScreen.java

class LossScreen extends Group {
private BitmapFont mediumFont, smallFont;
private GlyphLayout layout = new GlyphLayout();
private Texture backgroundTexture, buttonTexture;
private Rectangle tryAgainRect;
private int zIndex = 0;

LossScreen (int x, int y, AssetManager assetManager) {
    setBounds(x, y, TotallyNot2048.WIDTH, TotallyNot2048.HEIGHT);

    smallFont = assetManager.get("smallBoltFont.ttf", BitmapFont.class);
    mediumFont = assetManager.get("mediumFont.ttf", BitmapFont.class);

    backgroundTexture = Helper.createRoundedRectangleTexture(getWidth(), getHeight(), new Color(200f / 256f, 200f / 256f, 200f / 256f, .8f), 15);

    layout.setText(smallFont, "Try again");
    buttonTexture = Helper.createRoundedRectangleTexture(layout.width * 1.5f, layout.height * 3f, new Color(143f / 256f, 122f / 256f, 102f / 256f, 1f), 3);
    tryAgainRect = new Rectangle(getX() + getWidth() / 2 - buttonTexture.getWidth() / 2, getY() + getHeight() / 3, layout.width * 1.5f, layout.height * 3f);

    listenForInput();
}

private void listenForInput() {
    addListener(new InputListener() {

        @Override
        public boolean touchDown(InputEvent event, float x, float y, int pointer, int button) {
            if (pointer == Input.Buttons.LEFT)
                if (tryAgainRect.contains(TotallyNot2048.getMousePosInGame().x, TotallyNot2048.getMousePosInGame().y))
                    ((Board) getParent()).restartGame();
            return true;
        }
    });
}

public void setZIndex(int zIndex) {
    this.zIndex = zIndex;
}

public int getZIndex() {
    return zIndex;
}

@Override
public void draw(Batch batch, float parentAlpha) {
    super.draw(batch, parentAlpha);

    batch.setColor(getColor().r, getColor().g, getColor().b, getColor().a * parentAlpha);
    batch.draw(backgroundTexture, 0, 0);
    layout.setText(mediumFont, "Game over!");
    mediumFont.setColor(119f / 256f, 110f / 256f, 101f/ 256f, mediumFont.getColor().a);
    mediumFont.getColor().a = getColor().a;
    mediumFont.draw(batch, "Game over!", getWidth() / 2 - layout.width / 2, getHeight() / 2 + layout.height / 2);
    mediumFont.getColor().a = 1f;

    batch.draw(buttonTexture, getWidth() / 2 - buttonTexture.getWidth() / 2, getHeight() / 3);
    layout.setText(smallFont, "Try again");
    smallFont.setColor(new Color(245f / 256f, 241f / 256f, 237f / 256f, 1f * getColor().a));
    smallFont.draw(batch, "Try again", getWidth() / 2 - layout.width / 2, getHeight() / 3 + layout.height * 2);
    batch.setColor(getColor().r, getColor().g, getColor().b, 1f);
}

@Override
public void act(float delta) {
    super.act(delta);
}
}

In "board.java" I check whether other moves are available, if not, this code is called and an "isLost" variable is set to true which prevents user input, "lossScreen" is an actor that upon setting to visible automatically calls the draw method in the LossScreen class:

private void showLossScreen() {
    lossScreen.setVisible(true);
    lossScreen.addAction(Actions.fadeIn(.4f));
}

• Game logic

This is the game logic I use for moving the tiles, "isExists()" checks if the tile is not already about to be deleted (it is still present because it needs to show an animation), "setMoveParameters()" gives data to a "Tile" that will be used by the tile to show an animation. "tiles[]" is an array of Tile objects (Tile extends Actor). Besides genericMove and genericUpgrade, I have four methods for each move direction:

    private boolean genericMove(int i, int j, int move_x, int move_y) {
    if ((tiles[i + j] != null && !tiles[i + j].isExists()) || tiles[i + j] == null) {
        tiles[i + j] = new Tile(tiles[i], this);
        addActor(tiles[i + j]);
        tiles[i + j].setMoveParameters(move_x, move_y, tiles[i].getValue(), false);
        removeActor(tiles[i]);
        tiles[i].dispose();
        tiles[i] = null;
        return true;
    }
    return false;
}

private boolean genericUpgradeMove(int i, int j, int move_x, int move_y) {
    if (tiles[i + j] != null && tiles[i + j].isExists() && tiles[i] != null) {
        if (tiles[i + j].getValue() == tiles[i].getValue() && !tiles[i + j].isUpgradedThisTurn() && !tiles[i].isUpgradedThisTurn()) {
            updateScore(tiles[i + j].getValue() * 2);
            tiles[i + j].setMoveParameters(0, 0, tiles[i + j].getValue() * 2, false);
            Tile tile = new Tile(tiles[i], this);
            tempTiles.add(tile);
            tile.setMoveParameters(move_x, move_y, tiles[i].getValue(), true);
            addActor(tile);
            removeActor(tiles[i]);
            tiles[i].dispose();
            tiles[i] = null;
            return true;
        }
    }
    return false;
}

private boolean moveRight() {
    boolean tileMoved = false;
    for (int i = tiles.length - 2; i >= 0; i--) {
        if (tiles[i] != null && tiles[i].isExists()) {
            if ((i + 1) % BOARD_SIZE != 0) {
                tileMoved = genericMove(i, 1, MOVE_DISTANCE, 0);
                if (tileMoved) {
                    moveRight();
                    break;
                }

                tileMoved = genericUpgradeMove(i, 1, MOVE_DISTANCE, 0);
                if (tileMoved) {
                    moveRight();
                    break;
                }
            }
        }
    }
    return tileMoved;
}

These methods are preceded by one of four methods for each direction like:

private boolean tryMoveRight() {
    pause();
    updateIndices();
    if (moveRight()) {
        updateTileGraphics();
        addNewTile();
        checkState();
        return true;
    } else {
        resume();
        return false;
    }
}

"pause()" causes no new user input to be taken in, resume resumes user input, updateIndices() makes sure all tiles are in their right position, so if their did happen to be new move instructions given by the user, the tile is first put into the position it should have been to begin with. "updateTileGraphics()" uses the data given by "setMoveParameters()" for eachs Tile. "checkState()" checks if the game is lost or won. "moveUp()" returns true or false depending on whether one or more tiles were able to move / slide into each other.

• Making faster input possible

Currently, when the tiles are still showing animations, no new input is accepted, this results in not being able to play the game really fast, I haven't figured out a way to fix this without making the animations really fast.

These are my main questions, of course their are much more things I'm not certain of whether I used best practices, but I don't know if anyone is interested in reviewing the entire project.

Answer 1

• OOP question

  I have created new classes for a loss screen and win screen. They of course both only need to be instanced once, and they always look the same, should I not have used OOP for this?

When coding in Java you should always use OOP.

But OOP does not mean to split up code into random classes. Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility / separation of concerns
• same level of abstraction
• prefer polymorphism over branching
• KISS (Keep it simple (and) stupid)
• DRY (Don't repeat yourself)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")

In Java the classes concept supports this principles but OOP is not bound to classes (except polymorphism).

---

You create classes when there is different behavior, where behavior means communication with dependencies (aka calling methods on other objects) or calculations.

Your classes for a loss screen and win screen might have different behavior (ie. adding different components to the screen). Then having separate classes would be OK.

If they just display different strings in different colors/sizes there should be only one class with a parametrized constructor.

---

• Game logic

IMHO there a tree major problems with your code:

1. poor naming
2. code duplication
3. methods with status return and side effects.

Naming

Finding good names is the hardest part in programming. So always take your time to find good names.

Although you follow the general Java Naming Conventions your identifier names could be improved in several ways. The fact that you have to explain your methods in prose in your question indicates that.

• Choose names from the problem solution domain, not from the programming domain.

• Let method names start with a verb.
  Methods are actions on an object. Having a verb as the first name part improves the readability of your code greatly:
  gameLogic.genericMove() vs gameLogic.move()

• boolean variables and methods returning a boolean should be prefixed with is or has

• Do not fear to be verbose.
  There is no penalty for long identifier names. (actual IDEs even prevent you from typing them more than once via code completion...) So resist to artificially shorten them.

code duplication

You have similar code to move your tiles into different directions.

By applying OOP you could move the differing calculations in each of that methods into classes implementing the same custom interface. Then you'd have only one method doing the loops getting the interface type as parameter. The differences for each direction would be encapsulated in classes of their own implementing this interface (which could be called Direction).

Please see this answer for more details: Simplified version of 2048 game

This way you would safe 3/4 of the "similar code".

Methods with status return and side effects

Methods should either return a (calculated) value or change the state of an object. Your "logic" methods do both.

Looks like the main reason for this is that you do your logic twice: first for checking the possibility and second for actually doing the move.

You could choose a different approach where you do the move in a copy of the board and simply exchange the "real" board with the copy if the move succeeded. In this scenario the game would have a member variable hasMoveSucceeded:

private boolean move(Direction direction) {
    pause();
    updateIndices();
    moveInCopy(direction);
    if (hasMoveSucceded) {
        exchangeBoardWithCopy();
        addNewTile();
        checkState();
        return true;
    } else {
        //  resume(); // maybe obsolete?
        return false;
    }
}

---

I do not how I would implement the different for loops in a parameterized method, some of the for loops are ascending and others are descending. – The Coding Wombat

the interface:

interface Direction{
  int startValue();
  boolean hasNext(int loopIndex);
  int next(int loopIndex);
  int getX(int loopIndex);
  int getY(int loopIndex);
}

the directions as anonymous inner classes:

Direction right = new Direction(){
  @Override public int startValue(){
    return tiles.length - 2;
  }
  @Override public int hasNext(int loopIndex){
    return  loopIndex >= 0;    
  }
  @Override public int next(int loopIndex){
    return  loopIndex--;
  }
  @Override public int getX(int loopIndex){
    return loopIndex;
  }
  @Override public int getY(int loopIndex){
    return 1;
  }
}

---

next two values just guessed, hope you see the idea...

Direction left = new Direction(){
  @Override public int startValue(){
    return 0;
  }
  @Override public int hasNext(int loopIndex){
    return  loopIndex < tiles.length - 2;    
  }
  @Override public int next(int loopIndex){
    return  loopIndex++;
  }
  @Override public int getX(int loopIndex){
    return loopIndex;
  }
  @Override public int getY(int loopIndex){
    return 1;
  }
}

---

Direction down = new Direction(){
  @Override public int hasNext(int loopIndex){
    return  loopIndex < tiles.length - 2;    
  }
  @Override public int startValue(){
    return 0;
  }
  @Override public int next(int loopIndex){
    return  loopIndex++;
  }
  @Override public int getX(int loopIndex){
    return 1; 
  }
  @Override public int getY(int loopIndex){
    return loopIndex;
  }
}

the loop method

private void move(Direction direction) {
    tileMoved = false;
    for (int i = direction.startValue(); direction.hasNext(i); i=direction.next(i)) {
        if (tiles[i] != null && tiles[i].isExists()) {
            if ((i + 1) % BOARD_SIZE != 0) {
                tileMoved = genericMove(direction.getX(i), direction.getY(i), MOVE_DISTANCE, 0);
                if (tileMoved) {
                    move(direction);
                    break;
                }

                tileMoved = genericUpgradeMove(direction.getX(i), direction.getY(i), MOVE_DISTANCE, 0);
                if (tileMoved) {
                    move(direction);
                    break;
                }
            }
        }
    }
}