Simple Java Tetris game

Question

I have no IT background and taught myself Java and made a simple Tetris game. I compared my code on the internet with other tutorials. My first impression of those implementations is that they are very complex: Shape-classes, No-Shape-shape, rotation arrays etc.

The location of a shape in the grid is also the shape's central/rotation point. A shape is an array of 4 Points/coordinates relative to the shapes central/rotation point.

Is my code too simple? I would like some feedback.

Here is my code:

import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Component;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Graphics;
import java.awt.Point;
import java.awt.Toolkit;
import java.awt.event.ComponentAdapter;
import java.awt.event.ComponentEvent;
import java.awt.event.KeyAdapter;
import java.awt.event.KeyEvent;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Random;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JLayeredPane;
import javax.swing.JPanel;

public class Tetris extends JFrame implements Runnable {
    final private static String I = "i";
    final private static String O = "o";
    final private static String J = "j";
    final private static String L = "l";
    final private static String T = "t";
    final private static String S = "s";
    final private static String Z = "z";
    
    final private static HashMap SHAPE = new HashMap();
    final private static HashMap COLOR = new HashMap();
    
    final private int X = 10;
    final private int Y = 20;
    final private int SIZE = Toolkit.getDefaultToolkit().getScreenSize().height / 2 / Y;
    final private int MIN = 100;
    final private int MAX = 750;
    final private int DELAY = 5;
    
    final private JLabel PIECE = new JLabel() {
        @Override
        public void paint(Graphics g) {
            g.setColor((Color) COLOR.get(getText()));
            for (Point p : block) {
                g.fillRect((location.x + p.x) * SIZE, (location.y + p.y) * SIZE, SIZE, SIZE);
            }
        }
    };
    final private JLabel NEXT = new JLabel((String) SHAPE.keySet().toArray()[new Random().nextInt(SHAPE.size())]) {
        @Override
        public void paint(Graphics g) {
            g.setColor((Color) COLOR.get(getText()));
            for (Point p : (Point[]) SHAPE.get(getText())) {
                g.fillRect(getWidth() / 2 + (int) ((p.x - 0.5) * SIZE), (p.y + 2) * SIZE, SIZE, SIZE);
            }
        }
    };
    final private JLabel GAME_OVER = new JLabel("Game over", JLabel.CENTER);
    final private JLabel SCORE = new JLabel("0", JLabel.RIGHT);
    
    final private JButton START = new JButton("Start");
    
    private ArrayList<Color[]> grid = new ArrayList(Arrays.asList(new Color[Y][X]));
    
    private Point location;
    private Point[] block;

    public Tetris() {
        JLayeredPane board = new JLayeredPane();
        JPanel statusPanel = new JPanel(new BorderLayout());
            
        board.setOpaque(true);
        board.setBackground(Color.lightGray);
        board.add(new JPanel() {
            @Override
            public void paint(Graphics g) {
                for (int y = 0; y < Y; y++) {
                    for (int x = 0; x < X; x++) {
                        if (grid.get(y)[x] != null) {
                            g.setColor(grid.get(y)[x]);
                            g.fillRect(x * SIZE, y * SIZE, SIZE, SIZE);
                        }
                    }
                }
            }
        });
        board.add(PIECE, new Integer(1));
        board.add(GAME_OVER, new Integer(2));
        board.setPreferredSize(new Dimension(X * SIZE, Y * SIZE));
        for (Component c : board.getComponents()) {
            c.setSize(board.getPreferredSize());
        }

        statusPanel.add(SCORE, BorderLayout.NORTH);
        statusPanel.add(START, BorderLayout.CENTER);
        statusPanel.add(NEXT, BorderLayout.SOUTH);
        
        PIECE.setVisible(false);
        PIECE.addKeyListener(new KeyAdapter() {
            @Override
            public void keyPressed(KeyEvent e) {
                if (e.getKeyCode() == KeyEvent.VK_LEFT) {
                    blockXY(location.x - 1, location.y, block);
                } else if (e.getKeyCode() == KeyEvent.VK_RIGHT) {
                    blockXY(location.x + 1, location.y, block);
                } else if (e.getKeyCode() == KeyEvent.VK_DOWN) {
                    blockXY(location.x, location.y + 1, block);
                } else if (e.getKeyCode() == KeyEvent.VK_SPACE && !PIECE.getText().equals(O)) {
                    Point[] rotated = new Point[block.length];
                    int x = location.x;
                    for (int i = 0; i < rotated.length; i++) {
                        rotated[i] = new Point(block[i].y, -block[i].x);
                        x = Math.max(-rotated[i].x, Math.min(x, X - 1 - rotated[i].x));
                    }
                    blockXY(x, location.y, rotated);
                }
            }
        });
        PIECE.addComponentListener(new ComponentAdapter() {
            @Override
            public void componentShown(ComponentEvent e) {
                PIECE.requestFocus();
            }
        });
        
        GAME_OVER.setVisible(false);
        GAME_OVER.setFont(new Font(Font.DIALOG_INPUT, Font.BOLD, GAME_OVER.getHeight() / 12));
        
        SCORE.setFont(GAME_OVER.getFont());

        START.setFont(GAME_OVER.getFont());
        START.setBorder(null);
        START.setContentAreaFilled(false);
        START.setFocusPainted(false);
        START.addActionListener(e -> new Thread(this).start());

        NEXT.setPreferredSize(new Dimension(PIECE.getHeight() / 3, PIECE.getHeight() / 3));

        add(board, BorderLayout.WEST);
        add(statusPanel, BorderLayout.EAST);
        
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setResizable(false);
        setVisible(true);
        pack();
    }
    
    private synchronized boolean blockXY(int x, int y, Point[] block) {
        for (Point p : block) {
            if (x + p.x == -1 || x + p.x == X || y + p.y >= Y || (y + p.y > -1 && grid.get(y + p.y)[x + p.x] != null)) {
                return false;
            }
        }
        location = new Point(x, y);
        this.block = block;
        repaint();
        return true;
    }

    @Override
    public void run() {
        START.setVisible(false);
        grid = new ArrayList(Arrays.asList(new Color[Y][X]));        
        SCORE.setText("0");
        GAME_OVER.setVisible(false);
        
        String choice = I + O + J + L + T + S + Z;
        
        do {
            PIECE.setText(NEXT.getText());
            NEXT.setText(choice.split("")[new Random().nextInt(choice.length())]);
            location = new Point(X / 2, -2);
            block = (Point[]) SHAPE.get(PIECE.getText());                
            PIECE.setVisible(true);            

            do {
                try {
                    Thread.sleep(Math.max(MIN, MAX - Integer.parseInt(SCORE.getText()) * DELAY));
                } catch (Exception ex) { }
                PIECE.setVisible(blockXY(location.x, location.y + 1, block));
            } while (PIECE.isVisible());

            for (Point p : block) {
                try {
                    grid.get(location.y + p.y)[location.x + p.x] = (Color) COLOR.get(PIECE.getText());
                } catch (Exception ex) { 
                    GAME_OVER.setVisible(true);
                }
            }

            if (!GAME_OVER.isVisible()) {
                for (int y = 0; y < Y; y++) {
                    if (!Arrays.asList(grid.get(y)).contains(null)) {
                        grid.set(y, new Color[X]);
                        repaint();
                        try {
                            Thread.sleep(800);
                        } catch (Exception ex) { }
                        grid.add(0, grid.remove(y));
                        SCORE.setText("" + (Integer.parseInt(SCORE.getText()) + 1));
                    }
                }
                choice = (I + O + J + L + T + S + Z).replace(NEXT.getText(), NEXT.getText().replace(PIECE.getText(), ""));
            } else {
                try {
                    Thread.sleep(1200);
                } catch (Exception ex) { }
                START.setVisible(true);
                START.requestFocus();
            }
        } while (!GAME_OVER.isVisible());
    }
    
    public static void main(String[] args) {
        SHAPE.put(I, new Point[] {new Point(0, -2), new Point(0, -1), new Point(0, 0), new Point(0, 1)});
        SHAPE.put(O, new Point[] {new Point(0, 0), new Point(-1, 0), new Point(0, 1), new Point(-1, 1)});
        SHAPE.put(J, new Point[] {new Point(0, -1), new Point(0, 0), new Point(0, 1), new Point(-1, 1)});
        SHAPE.put(L, new Point[] {new Point(0, -1), new Point(0, 0), new Point(0, 1), new Point(1, 1)});
        SHAPE.put(T, new Point[] {new Point(-1, 0), new Point(0, 0), new Point(1, 0), new Point(0, 1)});
        SHAPE.put(S, new Point[] {new Point(1, 0), new Point(0, 0), new Point(0, 1), new Point(-1, 1)});
        SHAPE.put(Z, new Point[] {new Point(-1, 0), new Point(0, 0), new Point(0, 1), new Point(1, 1)});
        
        COLOR.put(I, Color.cyan);
        COLOR.put(O, Color.yellow);
        COLOR.put(J, Color.blue);
        COLOR.put(L, Color.orange);
        COLOR.put(T, Color.magenta);
        COLOR.put(S, Color.green);
        COLOR.put(Z, Color.red);
        
        new Tetris().setLocationRelativeTo(null);
    }
    
}

Answer 1

Is my code too simple?

We need to unpack that a little, because we're probably operating on different definitions of "simple". Your application has only one file and one top-level class. Whereas that is in one sense simple, it's not a very good idea as it hinders maintainability and legibility, and actually implies more complexity when it comes to cognitive load - understanding how your code is laid out and being able to modify and test it meaningfully. You should attempt to subdivide and modularise.

A battle-weary programmer's idea of simplicity is quite different, and involves smaller, more well-defined classes; better separation of concerns; etc.

On a more granular level:

Your single-letter constants are a code smell, and suggest that you're actually attempting to capture an enum of some kind.

SHAPE and COLOR shouldn't be capitalised I think; need <> type parameters; and it's important that they be initialised inline instead of outside of the instance, something like

    private final Map<String, Point[]> shapes = Map.ofEntries(
        entry(I, new Point[] {new Point( 0, -2), new Point( 0, -1), new Point(0, 0), new Point( 0, 1)}),
        entry(O, new Point[] {new Point( 0,  0), new Point(-1,  0), new Point(0, 1), new Point(-1, 1)}),
        entry(J, new Point[] {new Point( 0, -1), new Point( 0,  0), new Point(0, 1), new Point(-1, 1)}),
        entry(L, new Point[] {new Point( 0, -1), new Point( 0,  0), new Point(0, 1), new Point( 1, 1)}),
        entry(T, new Point[] {new Point(-1,  0), new Point( 0,  0), new Point(1, 0), new Point( 0, 1)}),
        entry(S, new Point[] {new Point( 1,  0), new Point( 0,  0), new Point(0, 1), new Point(-1, 1)}),
        entry(Z, new Point[] {new Point(-1,  0), new Point( 0,  0), new Point(0, 1), new Point( 1, 1)})
    );

    private final Map<String, Color> colors = Map.ofEntries(
        entry(I, Color.cyan),
        entry(O, Color.yellow),
        entry(J, Color.blue),
        entry(L, Color.orange),
        entry(T, Color.magenta),
        entry(S, Color.green),
        entry(Z, Color.red)
    );

The fact that colors and shapes have the same cardinality suggests that there's a class there that you need to capture, having members for points and color.

The expression grid.get(y)[x] needs to go into a variable for reuse.

Don't new Integer(1); just write 1.

This predicate:

if (x + p.x == -1 || x + p.x == X || y + p.y >= Y || (y + p.y > -1 && grid.get(y + p.y)[x + p.x] != null)) {
        

should be line-separated:

        if (x + p.x == -1
            || x + p.x == X
            || y + p.y >= Y
            || (y + p.y > -1 && grid.get(y + p.y)[x + p.x] != null)
        ) {

It's probably not a good idea for you to new Random() on the inside of run() in a loop. You should have a single Random instance, potentially as a member on the class but at the very least removed from that loop.

catch (Exception ex) is poison to a program's debug-ability. It needs to go away, and if you're concerned about a specific exception from the try block, catch that instead.

Answer 2

Separation of Concerns

You seem to have all the code in one class, which makes it easy in the beginning, but very hard the more functionality you add. Having everything in one place makes code less modifiable and less understandable, since so much is going on at once.

That's why there exist lots of patterns around separating different concerns so they can be modified independently of each other. I like MVVM. In MVVM you would have at least 3 classes: one for the logic, one for the UI and one connecting layer.

Long Methods

Your methods are very long with multiple levels of nesting. This can be difficult to read and understand.

I like to use these general rules of thumb:

• Is the method longer than 5 lines? -> Extract some of it.
• Does the method have more than one level of indention? E.g. nested ifs or loops? -> Extract until it only has one level indention.

Extracting code blocks into functions enables you do to give that block a name. This name should give a clear idea about the intent of the block, not the implementation. That way a reader does not have to read the method content to understand what the method is doing, only if they also want to know how it is achieving that.

For more in depth info you can view my post on codereview meta about readable code.