7
\$\begingroup\$

I made a 2048 game clone in Java for training, and I am trying to make it more efficient.

The problem I found is the paintComponent() method always resets all the graphics before doing anything, so I am forced to repaint all 4 rows of the game every time I want to repaint. Obviously I didn't want to override paint() and/or the update() method because I don't know how to correctly override them and it's not a good idea anyway.

The game works so far, but, before I proceed further, I am looking for ideas on how to only repaint the rows that get affected. For example, if the player presses and only the bottom two rows get affected and the top ones don't change at all, I only want to redraw the bottom rows for efficiency. I had this idea of having an array of 4 int elements where each element represents a row. If the is_moved flag becomes true I set that row in that array to equal 0. If that row didn't change, the index in the array for that row will be -1. But still, because of paintComponent() resetting everything beforehand, I have to every time I want to repaint(). I have to redraw all rows, even if some of them don't need to be repainted.

public class Game_Panel extends JPanel implements KeyListener
{
    //instance variables
    public TILE panel[][];
    public byte current_tiles;
    public boolean achieved_goal;

    public static final int default_start_A = 2;
    public static final int default_start_B = 4;
    public static final int HW = 489;
    public static final int seperation_length = 4;
    public static final int block_width = 119;
    public static final int block_center = 119>>1;
    public static final int RANDOM = 101;
    public static final byte ROWS_COLS = 4;
    public static final byte real_end = ROWS_COLS-1;
    public static final byte fake_end = 0;
    public static final byte left_increment = 1;
    public static final byte right_increment = -1;  

    //keyboard ascii numbers
    public static final byte LEFT = 37;
    public static final byte RIGHT = 39;
    public static final byte UP = 38;
    public static final byte DOWN = 40;

    //Colors of different numbers
    public static final Color BACKGROUND = new Color(187,173,160);
    public static final Color DEFAULT_TILE = new Color(204,192,179);
    public static final Color TWO = new Color(238,228,218);
    public static final Color FOUR = new Color(237,224,200);
    public static final Color EIGHT = new Color(242,177,121);
    public static final Color SIXTEEN = new Color(245,149,98);
    public static final Color THIRTYTWO = new Color(246,124,95);
    public static final Color SIXTYFOUR = new Color(246,94,59);
    public static final Color REMAINING = new Color(237,204,97);

    //x and y positions of the four possible places of a tile.
    public static final int JUMPS[] = {seperation_length,
                                      (block_width+seperation_length),
                                      ((block_width<<1)+seperation_length),
                                      (((block_width<<1)+block_width)+seperation_length)};

    public boolean is_moved = false;
    public final Font END = new Font("Lithograph", Font.BOLD, 50);
    public static final RenderingHints rh = new RenderingHints(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);

    public Game_Panel()
    {
        setBackground(BACKGROUND);
        setPreferredSize(new Dimension(HW,HW));
        setFocusable(true);
        requestFocusInWindow();
        addKeyListener(this);

        rh.put(RenderingHints.KEY_RENDERING, RenderingHints.VALUE_RENDER_QUALITY);      
        panel = new TILE[ROWS_COLS][ROWS_COLS];
        achieved_goal = false;

        //same as generate method, but thought it'd be a waste
        //to call it when we're initializing.
        Random row_col = new Random();
        byte row = (byte) row_col.nextInt(ROWS_COLS);
        byte col = (byte) row_col.nextInt(ROWS_COLS);
        int two_four = row_col.nextInt(RANDOM);

        if (two_four % 2 == 0)
        {
            panel[row][col] = new TILE(default_start_A);
        }
        else
        {
            panel[row][col] = new TILE(default_start_B);
        }
        current_tiles++;
    }

    public void paintComponent(Graphics g_first)
    {
        super.paintComponent(g_first);
        Graphics2D g = (Graphics2D) g_first;
        g.setRenderingHints(rh);
        for (byte row=0; row<ROWS_COLS; row++)
        {
            int Y_jump = JUMPS[row];    
            for (byte col=0; col<ROWS_COLS; col++)
            {
                int X_jump = JUMPS[col];

                if (panel[row][col] == null)
                {
                    g.setColor(DEFAULT_TILE);
                    g.fillRoundRect(X_jump, Y_jump, block_width, block_width, 80, 80);
                }
                else
                {
                    int value = panel[row][col].value;
                    JLabel temp = panel[row][col].LABEL;

                    if (value == 2)
                    {
                        g.setColor(TWO);
                        temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
                    }
                    else if (value == 4)
                    {
                        g.setColor(FOUR);
                        temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
                    }
                    else if (value == 8)
                    {
                        g.setColor(EIGHT);
                        temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
                    }
                    else if (value == 16)
                    {
                        g.setColor(SIXTEEN);
                        temp.setLocation(X_jump+block_center-28, Y_jump+block_center-23);
                    }
                    else if (value == 32)
                    {
                        g.setColor(THIRTYTWO);
                        temp.setLocation(X_jump+block_center-28, Y_jump+block_center-23);
                    }
                    else if (value == 64)
                    {
                        g.setColor(SIXTYFOUR);
                        temp.setLocation(X_jump+block_center-30, Y_jump+block_center-23);
                    }
                    else if (value < 1024)
                    {
                        g.setColor(REMAINING);
                        temp.setLocation(X_jump+block_center-45, Y_jump+block_center-20);
                    }
                    else
                    {
                        g.setColor(REMAINING);
                        temp.setFont(panel[row][col].big_number);
                        temp.setLocation(X_jump+block_center-45, Y_jump+block_center-15);
                    }

                    g.fillRoundRect(X_jump, Y_jump, block_width, block_width, 80, 80);
                    add(temp);
                }

            }
        }

        if (!achieved_goal)
        {
            if (current_tiles == 16)
            {
                boolean check = false;
                for (byte x=0; x<ROWS_COLS; x++)
                {
                    try{
                        byte y=0;
                        while  (y!=ROWS_COLS)
                        {
                            if (y+1 <= real_end && x+1 <= real_end)
                            {
                                if (panel[x][y].value == panel[x][y+1].value || panel[x][y].value == panel[x+1][y].value)
                                {
                                    check = true;
                                    break;
                                }
                                else
                                {
                                    y++;
                                }
                            }
                            else if (y+1 <= real_end)
                            {
                                if (panel[x][y].value == panel[x][y+1].value)
                                {
                                    check = true;
                                    break;
                                }
                                else
                                {
                                    y++;
                                }
                            }
                            else
                            {
                                if (panel[x][y].value == panel[x+1][y].value)
                                {
                                    check = true;
                                    break;
                                }
                                else
                                {
                                    y++;
                                }
                            }
                        }
                    }
                    catch (ArrayIndexOutOfBoundsException e){
                        break;
                    }
                    if (check)
                    {
                        break;
                    }
                }

                if (!check)
                {
                    System.out.println("YOU LOSE BAKA!!");
                    setEnabled(false);
                    try {
                        this.finalize();
                    } catch (Throwable e) {
                        e.printStackTrace();
                    }
                }
            }
        }
        else
        {
            System.out.println("YOU WIN!!");
            setEnabled(false);
            try {
                this.finalize();
            } catch (Throwable e) {
                e.printStackTrace();
            }
        }


    }

    //dummy methods
    public void keyReleased(KeyEvent e){}
    public void keyTyped(KeyEvent e) {}

    public void keyPressed(KeyEvent e)
    {
        is_moved=false;
        byte key = (byte)e.getKeyCode();
        if (key == LEFT)
        {
            is_moved = horizontal_pressed(real_end, left_increment);
        }
        else if (key == RIGHT)
        {
            is_moved = horizontal_pressed(fake_end, right_increment);
        }
        else if (key == UP)
        {
            panel = rotateRight(panel);
            is_moved = horizontal_pressed(fake_end, right_increment);
            panel = rotateLeft(panel);
        }
        else if (key == DOWN)
        {
            panel = rotateRight(panel);
            is_moved = horizontal_pressed(real_end,left_increment);
            panel = rotateLeft(panel);
        }
        Generate(is_moved);
        repaint();
    }

    public boolean horizontal_pressed(byte left_or_right, byte increment)
    {
        byte compare = (byte)(increment+left_or_right);
        byte which_end = (byte)(real_end-left_or_right);

        for (byte row=0; row<ROWS_COLS; row++)
        {
            shift_row(row,which_end,compare,increment);
        }

        //merge_row
        for (byte y=0; y<ROWS_COLS; y++)
        {
            byte x = which_end;
            while (x != compare && x != left_or_right)
            {
                if (panel[y][x] != null && panel[y][x+increment] != null && panel[y][x].value == panel[y][x+increment].value)
                {
                    panel[y][x].doubleValue();
                    remove(panel[y][x+increment].LABEL);
                    panel[y][x+increment] = null;
                    current_tiles--;
                    is_moved = true;
                    x = (byte)(x+(increment+increment));
                }
                else
                {
                    x = (byte)(x+increment);
                }
            }
            shift_row(y,which_end,compare,increment);
        }

        return is_moved;

    }

    public void shift_row(byte row, byte which_end, byte compare, byte increment)
    {
        ArrayList<TILE> temp_row = new ArrayList<TILE>();
        byte col;
        for (col = which_end; col!=compare; col = (byte)(col+increment))
        {
            if (panel[row][col] != null)
            {
                temp_row.add(panel[row][col]);
            }
        }

        byte next = 0;
        for (col=which_end; col!=compare; col= (byte)(col+increment))
        {
            try {
                if (temp_row.get(next) != panel[row][col])
                {
                    is_moved = true;
                    panel[row][col] = temp_row.get(next);
                }
            }
            catch (IndexOutOfBoundsException E) {
                panel[row][col] = null;
            }

            next++;
        }
    }

    public void Generate(boolean is_moved)
    {
        if (is_moved)
        {
            Random row_col = new Random();
            byte row = (byte) row_col.nextInt(ROWS_COLS);
            byte col = (byte) row_col.nextInt(ROWS_COLS);
            int two_four = row_col.nextInt(RANDOM);

            if (two_four % 2 == 0)
            {
                if (panel[row][col] == null)
                {
                    panel[row][col] = new TILE(default_start_A);
                    current_tiles++;
                }
                else
                {
                    Generate(is_moved);
                }
            }
            else
            {
                if (panel[row][col] == null)
                {
                    panel[row][col] = new TILE(default_start_B);
                    current_tiles++;
                }
                else
                {
                    Generate(is_moved);
                }
            }
        }
    }

   public TILE[][] rotateLeft(TILE image[][])
   {
      TILE new_image[][] = new TILE[ROWS_COLS][ROWS_COLS];

      for (int y=0; y<ROWS_COLS; y++) 
      {
          for (int x=0; x<ROWS_COLS; x++) 
          {
              new_image[x][y] = image[y][real_end - x];
          }
      }

      return new_image;
   }

    public TILE[][] rotateRight(TILE image[][])
    {
       TILE new_image[][] = new TILE[ROWS_COLS][ROWS_COLS];
       for (int y=0; y<ROWS_COLS; y++)   
       {
           for (int x=0; x<ROWS_COLS; x++)    
           {
               new_image[x][real_end - y] = image[y][x];  
           }
       }

       return new_image;
    }    

    class TILE
    {
        public int value;
        public JLabel LABEL;

        public final Font font = new Font("Lithograph", Font.BOLD, 50);
        public final Font big_number = new Font("Lithograph", Font.BOLD, 35);
        public final Color DEFAULT = new Color(119,110,101);
        public final Color REMAINING = new Color(249,246,242);

        public TILE(int value)
        {
            this.value = value;
            LABEL = new JLabel(Integer.toString(value));
            LABEL.setSize(40,40);
            LABEL.setFont(font);
            LABEL.setForeground(DEFAULT);
        }

        public void doubleValue()
        {
            value = value<<1;
            LABEL.setText(Integer.toString(value));
            if (value > 4)
            {
                LABEL.setForeground(REMAINING);
            }
            if (value == 2048)
            {
                Game_Panel.this.achieved_goal = true;
            }
        }
    }

    public static void main(String argc[])
    {
        System.setProperty("awt.useSystemAAFontSettings","on"); 
        System.setProperty("swing.aatext", "true"); 

        JFrame window = new JFrame();
        window.setSize(new Dimension(HW,HW));
        window.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        window.setResizable(false);
        window.setContentPane(new Game_Panel());
        window.pack();
        window.setVisible(true);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It seems like you have misunderstood what this site is about. We help with improving the cleanliness of existing, working code. We are not the site to ask for help in fixing or changing what your code does (i.e. repainting everything when it only needs to repaint parts of it). We would love to help you clean up your code though, as there seems to be some code duplication and other bigger or smaller code smells in it. \$\endgroup\$ – Simon Forsberg May 20 '14 at 8:24
  • \$\begingroup\$ Simon .. getting a feedback on how to improve performance is and could be an outcome of a code review \$\endgroup\$ – Deian Jul 21 '16 at 18:50
7
\$\begingroup\$

Since Simon mentioned that re-writing the output is off-topic here (plus that's not really my area of expertise), I'll just do a general code review.

And FWIW: For a simple project like this, you don't need to do such an optimization.


public class Game_Panel extends JPanel implements KeyListener

Don't put everything into one big class. Use an MVC pattern you should have (at least):

  • a class for the game logic completely independent from IO and Swing (Model)
  • an extension of JPanel for the output (View)
  • a class which basically represents the window/application and which handles raw input and lets the first two classes communicate (Controller with a bit of View)
  • possibly a class just consisting of the main function

public static final int default_start_A = 2;
public static final int default_start_B = 4;

public static final Color TWO = new Color(238,228,218);
public static final Color FOUR = new Color(237,224,200);
public static final Color EIGHT = new Color(242,177,121);

Any time you find yourself numbering/lettering variables, or creating any other similar sequence of variables, you are doing something wrong. These should be arrays or lists of some kind.

All constants (static final variables) should have names consisting only of capital letters.


 public static final int block_center = 119>>1;

It's completely unnecessary to use bit-shifting for multiplication/division by two here.


public static final byte real_end = ROWS_COLS-1;
public static final byte fake_end = 0;

I don't get the meaning of these to variable names.


//same as generate method, but thought it'd be a waste
//to call it when we're initializing.

What the? It what way would that be a "waste"? One of the top principles of good programming is DRY (Don't Repeat Yourself). There is absolutely no reason for this.


Random row_col = new Random();

Don't repeatedly create a Random object each time you need a random number. Create a single one at the start of the program and use that.

(That's all for now. There are still much more problems. Maybe I'll come back later.)

\$\endgroup\$
  • 1
    \$\begingroup\$ thanks so much for the review I really appreciate it, I will fix everything you talked about! \$\endgroup\$ – lukajenga May 20 '14 at 19:26
7
\$\begingroup\$

Please check your naming conventions. I'm not sure from which language you are coming, but Java is usually using no under_scores but camelCase for variable and method names. Especially if you pick one notation don't switch in between of a single program (horizontal_pressed vs. rotateRight). It is also a common pattern to start the method names with the verb. On the other hand, classes are usually nouns (Generate?) and are never written in all-caps (TILE). All-caps words are usually constants (final).

Although naming seems to be only a minor issue, it requires a lot of effort for the reader to understand your code, if she is used to a common pattern for a specific language.

RoToRa mentioned already not to run any premature optimization. Usually any optimization will lead to worse readability. E.g. understanding byte left_or_right requires reading the whole method (left=1|0|-1?). The meaning of boolean isLeft is obvious.

\$\endgroup\$
7
\$\begingroup\$

Magic Numbers: paintComponent() has a lot of hard coded values. Changing those to named constants will make the code clearer to a reader as well as making UI tweaks easier. For example, if you want to change the size of each square, you have to find all of the locations where you draw one. Find & Replace 80 to 100 might work, but you might also use 80 somewhere else to mean something else. Where as if you define a constant, you just need to change the value.


Repeated Code: When you check the value for the specific square, you do the same operation with different values. Instead, you can write one function that does the same operation and takes the values as arguments. That way, if you find a bug, you only need to change it in one place.


Long methods and Deeply nested code: You should be breaking your methods into smaller pieces. paitComponent() is over 150 lines long and at one point is nested 7 levels deep in brackets. This makes the code harder to read and harder to maintain. Find section that does encompasses a block of code and pull it out into a different function.


Switches Instead of Long If Chains:

if (value == 2)
{
    g.setColor(TWO);
    temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
}
else if (value == 4)
{
    g.setColor(FOUR);
    temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
}
//....

This is exactly what a switch statement is designed to do.

switch(value) {
    case 2:
        g.setColor(TWO);
        temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
        break;
    case 4:
        g.setColor(FOUR);
        temp.setLocation(X_jump+block_center-18, Y_jump+block_center-20);
        break;
    //....
}

The switch statement will look even cleaner once you extract the repeated code into a function as I mentioned earlier.

\$\endgroup\$
  • \$\begingroup\$ all your criticisms are so true I never realized them myself I feel like a moron lol, really much thanks, I appreciate it!! :) will put them in my mind on my current project, also, is there any other difference between switch and if statements other than ease of readability of code? \$\endgroup\$ – lukajenga May 21 '14 at 20:18
  • 2
    \$\begingroup\$ @lukajenga: I suspect that the compiler generates different bytecode and that a switch would be slightly faster. However, if that is true, it's highly unlikely that the bottle neck of you application is if statement branching. Using a profiler is the best way to see what is can be optimized when speed has become an issue. \$\endgroup\$ – unholysampler May 21 '14 at 20:27
  • 2
    \$\begingroup\$ @lukajenga Believe me, we all started this way, we are just some failures ahead :D \$\endgroup\$ – mheinzerling May 22 '14 at 3:53
1
\$\begingroup\$

Here is an optimization for you - don't use exceptions to drive your logic!

Performance wise - throwing exception in java is very costly. JVM needs to create an exception context and whole bunch of other stuff.

Change your logic to use if conditions

and don't rely on exceptions like:

    catch (ArrayIndexOutOfBoundsException e){
          break;
    }

or

    catch (IndexOutOfBoundsException E) {
        panel[row][col] = null;
    }
\$\endgroup\$

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.