6
\$\begingroup\$

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;
}    
\$\endgroup\$

migrated from stackoverflow.com Feb 3 '15 at 4:29

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ i would like to commment on one line above is that :slideTile.remove(j); is removing object inside for loop and every time its also checking the size of that object, i think you should use iterator instead \$\endgroup\$ – KOTIOS Feb 3 '15 at 4:37
  • 1
    \$\begingroup\$ @diva feel free to develop this idea into a perfectly acceptable (and appreciated) Code Review answer, with just a few more sentences and a bit of code to illustrate your point :) \$\endgroup\$ – Mathieu Guindon Feb 3 '15 at 5:00
  • \$\begingroup\$ @diva i'm confused as to what iterator does and how to use it. As far as I know the way i wrote it works. \$\endgroup\$ – Omar A. Khawaja Feb 3 '15 at 5:13
  • 1
    \$\begingroup\$ You have a syntax error in your code. The brackets don't match. There is an additional closing bracket at the end. Also please format your code properly as it is hard to read, otherwise. \$\endgroup\$ – barq Feb 3 '15 at 8:02
  • \$\begingroup\$ @barq on Code Review we like to review the code exactly as it is in OP's IDE - if it means broken indentation and an unreadable mess, so be it: reviewers can address these points in their answers! \$\endgroup\$ – Mathieu Guindon Feb 5 '15 at 1:41
6
\$\begingroup\$

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;
    }
    
\$\endgroup\$
  • 1
    \$\begingroup\$ That ternary looks atrocious. I agree with merging the identical parts, but why not use four ifs instead of the ternary? \$\endgroup\$ – Hosch250 Feb 4 '15 at 18:06
  • 1
    \$\begingroup\$ @Hosch250 That is also a good option. \$\endgroup\$ – TheCoffeeCup Feb 4 '15 at 18:08
  • \$\begingroup\$ Yeah, I'd generally avoid using the ternary operator when there are more than three conditions to check. It's unsightly. Though the comments within definitely don't help this case. \$\endgroup\$ – Legato Feb 5 '15 at 4:22
  • \$\begingroup\$ Thanks for your help, I really appreciate you taking the time to do this. I went back and I fixed the indentation, but I still feel lost on merging code. I'm a little confused with the if statements and int temp suggestion. Don't I already have 4 different if statements for each case? \$\endgroup\$ – Omar A. Khawaja Feb 6 '15 at 0:14
  • \$\begingroup\$ @Omar All your code is similar, except some values. Use int temp for the temporary variable to replace your different values. @Pixelstix also has some good suggestions. \$\endgroup\$ – TheCoffeeCup Feb 6 '15 at 5:11
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Note: This is an example of using enums to implement a Strategy pattern. See link for another example. \$\endgroup\$ – Pixelstix Feb 5 '15 at 22:07
  • \$\begingroup\$ Thanks for your help. I really appreciate it. I have to do some more research on how enums work, my Java didn't cover enums too much. \$\endgroup\$ – Omar A. Khawaja Feb 6 '15 at 0:12
  • \$\begingroup\$ If you come to the conclusion this is a reasonable way of factoring out the code into multiple pieces, I'd appreciate if you mark this as the answer. \$\endgroup\$ – Pixelstix Feb 10 '15 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.