2048 Java clone

Question

I'm making a 2048 clone and my method to move and merge tiles is really long. I've heard of helper methods but I'm a little lost as to how I should use them. I feel like my code is a little too long and can be made shorter but I don't know how. What can I do to make it shorter and cleaner? I'll also take any constructive criticism on anything else part of the method.

 public boolean move(Direction direction)
{
boolean didMove = false;
int multiplier = 2;
if (direction == Direction.RIGHT) {

    //int i represents rows; int j represents columns
    for (int i = 0; i < GRID_SIZE; i++ ) {
    ArrayList<Integer> slideTile = new ArrayList<>();

    //Copying row w/o 0s  to ArrayList
    for (int j = 0; j < GRID_SIZE; j++) {
        if(grid[i][j] != 0) {
        slideTile.add(grid[i][j]);
        }
    }

    //Merging tiles that are next to each other if
    //they are equal and updating score.
    for (int j = slideTile.size()-1; j > 0; j--) {
        if (slideTile.get(j).equals(slideTile.get(j-1))) {
        slideTile.set(j, multiplier*slideTile.get(j-1));
        score = score + multiplier*slideTile.get(j-1);
        slideTile.set(j-1, 0);
        }
    }

    //Removing 0s that may have resulted from merging
    for (int j = 0; j < slideTile.size(); j++ ) {
        if (slideTile.get(j) == 0) {
            slideTile.remove(j);
        }
    }

    //Adding 0s to beginning of ArrayList to slide tiles right
    int size = slideTile.size();
    for (int j = 0; j < (GRID_SIZE - size); j++){
        slideTile.add(0, 0);
    }

    //Copying ArrayList row back to game grid.
    for (int j = 0; j < GRID_SIZE; j++) {
        this.grid[i][j] = slideTile.get(j);
    }
    }
    didMove = true;
    }

if (direction == Direction.LEFT) {

    //int i represents rows; int j represents columns
    for (int i = 0; i < GRID_SIZE; i++ ) {
    ArrayList<Integer> slideTile = new ArrayList<>();

    //Copying row w/o 0s  to ArrayList
    for (int j = 0; j < GRID_SIZE; j++) {
        if(grid[i][j] != 0) {
        slideTile.add(grid[i][j]);
        }
    }

    //Merging tiles that are next to each other if
    //they are equal and updating score.        
    for (int j = 0; j < slideTile.size()-1; j++) {
        if (slideTile.get(j).equals(slideTile.get(j+1))) {
        slideTile.set(j, multiplier*slideTile.get(j+1));
        score = score + multiplier*slideTile.get(j+1);
        slideTile.set(j+1, 0);
        }
    }

    //Removing 0s that may have resulted from merging
    for (int j = 0; j < slideTile.size(); j++ ) {
        if (slideTile.get(j) == 0) {
            slideTile.remove(j);
        }
    }

    //Adding 0s to end of ArrayList to slide tiles left
    int size = slideTile.size();
    for (int j = 0; j < (GRID_SIZE - size); j++){
        slideTile.add(0);
    }

    //Copying ArrayList row back to game grid.
    for (int j = 0; j < GRID_SIZE; j++) {
        this.grid[i][j] = slideTile.get(j);
    }
    }
    didMove = true;
    }

if (direction == Direction.UP) {

    //int i represents rows; int j represents columns
    for (int j = 0; j < GRID_SIZE; j++ ) {
    ArrayList<Integer> slideTile = new ArrayList<>();

    //Copying row w/o 0s  to ArrayList
    for (int i = 0; i < GRID_SIZE; i++) {
        if ( grid [i][j] != 0 )
        slideTile.add(grid[i][j]);

    }

    //Merging tiles that are next to each other if
    //they are equal and updating score.        
    for (int i = 0; i < slideTile.size()-1; i++) {
        if (slideTile.get(i).equals(slideTile.get(i+1))) {
        slideTile.set(i, multiplier*slideTile.get(i+1));
        System.out.println(slideTile.get(i));
        score = score + multiplier*slideTile.get(i+1);
        slideTile.set(i+1, 0);
        }
    }

    //Removing 0s that may have resulted from merging
    for (int i = 0; i < slideTile.size(); i++ ) {
        if (slideTile.get(i) == 0) {
            slideTile.remove(i);
        }
    }

    //Adding 0s to end of ArrayList to slide tiles up
    int size = slideTile.size();
    for (int i = 0; i < (GRID_SIZE - size); i++){
        slideTile.add(0);
    }

    //Copying ArrayList row back to game grid.
    for (int i = 0; i < GRID_SIZE; i++) {
        this.grid[i][j] = slideTile.get(i);
    }

    }
    didMove = true;
    }

if (direction == Direction.DOWN) {

    //int i represents rows; int j represents columns
    for (int j = 0; j < GRID_SIZE; j++ ) {
    ArrayList<Integer> slideTile = new ArrayList<>();


    //Copying row w/o 0s  to ArrayList
    for (int i = 0; i < GRID_SIZE; i++) {
        if(grid[i][j] != 0) {
        slideTile.add(grid[i][j]);
        }
    }

    //Merging tiles that are next to each other if
    //they are equal and updating score.    
    for (int i = slideTile.size()-1; i > 0; i--) {
        if (slideTile.get(i).equals(slideTile.get(i-1))) {
        slideTile.set(i, multiplier*slideTile.get(i-1));
        score = score + multiplier*slideTile.get(i-1);
        slideTile.set(i-1, 0);
        }
    }

    //Removing 0s that may have resulted from merging
    for (int i = 0; i <slideTile.size(); i++ ) {
        if (slideTile.get(i) == 0) {
            slideTile.remove(i);
        }
    }

    //Adding 0s to end of ArrayList to slide tiles up
    int size = slideTile.size();
    for (int i = 0; i < (GRID_SIZE - size); i++){
        slideTile.add(0, 0);
    }

    //Copying ArrayList row back to game grid.
    for (int i = 0; i < GRID_SIZE; i++) {
        this.grid[i][j] = slideTile.get(i);
    }
    }
    didMove = true;
    }          
return didMove;
}    

Answer 1

Just some bad practices:

Indentation

Your indentation is a mess. It should look like this:

public boolean move(Direction direction) {
    boolean didMove = false;
    int multiplier = 2;
    if (direction == Direction.RIGHT) {

        // int i represents rows; int j represents columns
        for (int i = 0; i < GRID_SIZE; i++) {
            ArrayList<Integer> slideTile = new ArrayList<>();

            // Copying row w/o 0s to ArrayList
            for (int j = 0; j < GRID_SIZE; j++) {
                if (grid[i][j] != 0) {
                    slideTile.add(grid[i][j]);
                }
            }

            // Merging tiles that are next to each other if
            // they are equal and updating score.
            for (int j = slideTile.size() - 1; j > 0; j--) {
                if (slideTile.get(j).equals(slideTile.get(j - 1))) {
                    slideTile.set(j, multiplier * slideTile.get(j - 1));
                    score = score + multiplier * slideTile.get(j - 1);
                    slideTile.set(j - 1, 0);
                }
            }

            // Removing 0s that may have resulted from merging
            for (int j = 0; j < slideTile.size(); j++) {
                if (slideTile.get(j) == 0) {
                    slideTile.remove(j);
                }
            }

            // Adding 0s to beginning of ArrayList to slide tiles right
            int size = slideTile.size();
            for (int j = 0; j < (GRID_SIZE - size); j++) {
                slideTile.add(0, 0);
            }

            // Copying ArrayList row back to game grid.
            for (int j = 0; j < GRID_SIZE; j++) {
                this.grid[i][j] = slideTile.get(j);
            }
        }
        didMove = true;
    }

    if (direction == Direction.LEFT) {

        // int i represents rows; int j represents columns
        for (int i = 0; i < GRID_SIZE; i++) {
            List<Integer> slideTile = new ArrayList<>();

            // Copying row w/o 0s to ArrayList
            for (int j = 0; j < GRID_SIZE; j++) {
                if (grid[i][j] != 0) {
                    slideTile.add(grid[i][j]);
                }
            }

            // Merging tiles that are next to each other if
            // they are equal and updating score.
            for (int j = 0; j < slideTile.size() - 1; j++) {
                if (slideTile.get(j).equals(slideTile.get(j + 1))) {
                    slideTile.set(j, multiplier * slideTile.get(j + 1));
                    score = score + multiplier * slideTile.get(j + 1);
                    slideTile.set(j + 1, 0);
                }
            }

            // Removing 0s that may have resulted from merging
            for (int j = 0; j < slideTile.size(); j++) {
                if (slideTile.get(j) == 0) {
                    slideTile.remove(j);
                }
            }

            // Adding 0s to end of ArrayList to slide tiles left
            int size = slideTile.size();
            for (int j = 0; j < (GRID_SIZE - size); j++) {
                slideTile.add(0);
            }

            // Copying ArrayList row back to game grid.
            for (int j = 0; j < GRID_SIZE; j++) {
                this.grid[i][j] = slideTile.get(j);
            }
        }
        didMove = true;
    }

    if (direction == Direction.UP) {

        // int i represents rows; int j represents columns
        for (int j = 0; j < GRID_SIZE; j++) {
            ArrayList<Integer> slideTile = new ArrayList<>();

            // Copying row w/o 0s to ArrayList
            for (int i = 0; i < GRID_SIZE; i++) {
                if (grid[i][j] != 0)
                    slideTile.add(grid[i][j]);

            }

            // Merging tiles that are next to each other if
            // they are equal and updating score.
            for (int i = 0; i < slideTile.size() - 1; i++) {
                if (slideTile.get(i).equals(slideTile.get(i + 1))) {
                    slideTile.set(i, multiplier * slideTile.get(i + 1));
                    System.out.println(slideTile.get(i));
                    score = score + multiplier * slideTile.get(i + 1);
                    slideTile.set(i + 1, 0);
                }
            }

            // Removing 0s that may have resulted from merging
            for (int i = 0; i < slideTile.size(); i++) {
                if (slideTile.get(i) == 0) {
                    slideTile.remove(i);
                }
            }

            // Adding 0s to end of ArrayList to slide tiles up
            int size = slideTile.size();
            for (int i = 0; i < (GRID_SIZE - size); i++) {
                slideTile.add(0);
            }

            // Copying ArrayList row back to game grid.
            for (int i = 0; i < GRID_SIZE; i++) {
                this.grid[i][j] = slideTile.get(i);
            }

        }
        didMove = true;
    }

    if (direction == Direction.DOWN) {

        // int i represents rows; int j represents columns
        for (int j = 0; j < GRID_SIZE; j++) {
            ArrayList<Integer> slideTile = new ArrayList<>();

            // Copying row w/o 0s to ArrayList
            for (int i = 0; i < GRID_SIZE; i++) {
                if (grid[i][j] != 0) {
                    slideTile.add(grid[i][j]);
                }
            }

            // Merging tiles that are next to each other if
            // they are equal and updating score.
            for (int i = slideTile.size() - 1; i > 0; i--) {
                if (slideTile.get(i).equals(slideTile.get(i - 1))) {
                    slideTile.set(i, multiplier * slideTile.get(i - 1));
                    score = score + multiplier * slideTile.get(i - 1);
                    slideTile.set(i - 1, 0);
                }
            }

            // Removing 0s that may have resulted from merging
            for (int i = 0; i < slideTile.size(); i++) {
                if (slideTile.get(i) == 0) {
                    slideTile.remove(i);
                }
            }

            // Adding 0s to end of ArrayList to slide tiles up
            int size = slideTile.size();
            for (int i = 0; i < (GRID_SIZE - size); i++) {
                slideTile.add(0, 0);
            }

            // Copying ArrayList row back to game grid.
            for (int i = 0; i < GRID_SIZE; i++) {
                this.grid[i][j] = slideTile.get(i);
            }
        }
        didMove = true;
    }
    return didMove;
}

Other

1. ArrayList<Integer> slideTile = new ArrayList<>();

   Declaring with the interface type is better, unless you specifically want a method in the ArrayList. Also, don't use ArrayList unless you know the size of the List, and you know it will stay like so. Since ArrayList uses an array for storage, when you add something to the List that exceeds its size, it will create a new array and move everything into that one. Also, when you remove something, ArrayList has to move all the objects after the removed one 1 space back.

   But now to the point. Use a LinkedList instead.

2. Some of your lines are tightly spaced. Give them more space:

   slideTile.set(j, multiplier*slideTile.get(j-1)); to:

   slideTile.set(j, multiplier * slideTile.get(j - 1));

3. Much of your code is duplicated. Try merging them together and put this in the place where it is different:

   direction == Direction.RIGHT ? /* if it is Direction.RIGHT, otherwise: */ : (direction == Direction.LEFT ? /* if it is Direction.LEFT, otherwise: */ : (direction == Direction.UP ? /* if it is Direction.UP, otherwise: */ : /* It's Direction.DOWN. Do what you have to do.*/))

   (I know it looks horrible, but it is MUCH shorter than doing the almost same thing 4 times)

   Replace the comments with what you have to do, or:

   int temp;
   if(direction == Direction.RIGHT) {
       // temp = whatever;
   } else if(direction == Direction.LEFT) {
       // temp = whatever;
   } else if(direction == Direction.UP) {
       // temp = whatever;
   } else {
       // It's Direction.DOWN
       // temp = whatever;
   }
   

Answer 2

Manny Meng had lots of good suggestions. I have no idea about what this 2048 game is, but here are suggestions for changing the structure to eliminate duplicate code.

ETA: One quick change is that you had comments that i is the row and j is the column. Why not just use "row" and "col" as variable names?

You have four long cases that look very similar to each other. One "Code Smell" rule is that explicit case analysis is almost always a sign that you're doing something wrong.

The first major thing I see is the algorithm for computing the score. There are 2 variants for whether you're going forward through the slideTile list or backwards. I separated that out into a SlideDirection enum:

public enum SlideDirection
{
    FORWARD {
        @Override public int slide(List<Integer> slideTile, int multiplier)
        {
            int score = 0;
            for (int idx = 0; idx < slideTile.size() - 1; idx++)
            {
                if (slideTile.get(idx).equals(slideTile.get(idx + 1)))
                {
                    slideTile.set(idx, multiplier * slideTile.get(idx + 1));
                    score += multiplier * slideTile.get(idx + 1);
                    slideTile.set(idx + 1, 0);
                }
            }
            return score;
        }
    },
    BACKWARD {
        @Override public int slide(List<Integer> slideTile, int multiplier)
        {
            int score = 0;
            for (int idx = slideTile.size() - 1; idx > 0; idx--)
            {
                if (slideTile.get(idx).equals(slideTile.get(idx - 1)))
                {
                    slideTile.set(idx, multiplier * slideTile.get(idx - 1));
                    score += multiplier * slideTile.get(idx - 1);
                    slideTile.set(idx - 1, 0);
                }
            }
            return score;
        }
    };
    public abstract int slide(List<Integer> slideTile, int multiplier);
}

The second thing I see is the looping, whether you're going horizontal or vertical. I also put that into an Orientation enum. Notice that the move method takes in the SlideDirection to do whatever version of slide() is appropriate!

public enum Orientation
{
    Vertical {
        @Override public int move(SlideDirection direction, int[][]grid, int multiplier)
        {
            int score = 0;

            for (int col = 0; col < GRID_SIZE; col++)
            {
                ArrayList<Integer> slideTile = new ArrayList<>();

                // Copying col w/o 0s to ArrayList
                for (int row = 0; row < GRID_SIZE; row++)
                    if (grid[row][col] != 0)
                        slideTile.add(grid[row][col]);

                // Merging tiles that are next to each other if
                // they are equal and updating score.
                score += direction.slide(slideTile, multiplier);

                cleanUpSlideTile(slideTile);

                // Copying ArrayList col back to game grid.
                for (int row = 0; row < GRID_SIZE; row++)
                    grid[row][col] = slideTile.get(row);
            }
            return score;
        }
    },
    Horizontal {
        @Override public int move(SlideDirection direction, int[][]grid, int multiplier)
        {
            int score = 0;

            for (int row = 0; row < GRID_SIZE; row++)
            {
                List<Integer> slideTile = new ArrayList<>();

                // Copying row w/o 0s to ArrayList
                for (int col = 0; col < GRID_SIZE; col++)
                    if (grid[row][col] != 0)
                        slideTile.add(grid[row][col]);

                score += direction.slide(slideTile, multiplier);

                cleanUpSlideTile(slideTile);

                // Copying ArrayList row back to game grid.
                for (int col = 0; col < GRID_SIZE; col++)
                    grid[row][col] = slideTile.get(col);
            }
            return score;
        }
    };

    /**
     * Merging tiles that are next to each other if they are equal and
     * @param direction
     * @param grid
     * @param multiplier
     * @return
     */
    public abstract int move(SlideDirection direction, int[][]grid, int multiplier);

    private static void cleanUpSlideTile(List<Integer> slideTile)
    {
        // Removing 0s that may have resulted from merging
        for (int idx = 0; idx < slideTile.size(); idx++)
            if (slideTile.get(idx) == 0)
                slideTile.remove(idx);

        // Adding 0s to beginning of ArrayList to slideDirection tiles right
        int size = slideTile.size();
        for (int idx = 0; idx < (GRID_SIZE - size); idx++)
            slideTile.add(0, 0);
    }
}

Now your Direction enum will be made up of those two enums:

public enum Direction
{
    RIGHT(Orientation.Horizontal, SlideDirection.FORWARD),
    LEFT(Orientation.Horizontal, SlideDirection.BACKWARD),
    UP(Orientation.Vertical, SlideDirection.FORWARD),
    DOWN(Orientation.Vertical, SlideDirection.BACKWARD);

    private Orientation orientation;
    public Orientation getOrientation() { return orientation; }

    private SlideDirection slideDirection;
    public SlideDirection getSlideDirection() { return slideDirection; }

    Direction(Orientation orientation, SlideDirection slide)
    {
        this.orientation = orientation;
        this.slideDirection = slide;
    }
}

Then your move function looks something like this:

/**
* Return the score for the given direction.
* @param direction
* @return score
*/
public int move(Direction direction)
{
    int multiplier = 2;

    // TODO: Initialize the grid with values
    int[][] grid = new int[GRID_SIZE][];
    for (int i = 0; i < GRID_SIZE; i++)
        grid[i] = new int[GRID_SIZE];

    int score = direction.getOrientation().move(direction.getSlideDirection(), grid, multiplier);

    return score;
}

There are probably a few additional cleanups that could be done, better names, some changing of responsibilities, and unit tests would help verify the behavior, but it would be better to see where all of this sits within the rest of the code.