8
\$\begingroup\$

This is a simulation attempt of an "old-style game", where a "car" (nothing more than a red rectangle) must dodge some "obstacles" (blue squares) falling from the upper edge of the frame. If an obstacle hits the car, the game stops.

The app works fine, but since this is my first project involving painting and moving objects in Swing, I would appreciate the critical opinion of some expert, as I'm sure there is plenty of space for code improvement/optimization.

What do you think about the code? What should have I done in a different/better way? What are your recommendations in terms of optimization?

public class Little_car_frame extends JFrame {

    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    Little_car_frame frame = new Little_car_frame();
                    frame.setVisible(true);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        });
    }

    public Little_car_frame() {
        setResizable(false);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setBounds(100, 100, 450, 300);
        getContentPane().add(new Little_car_panel());
    }

}

public class Little_car_panel extends JPanel implements KeyListener {

    private Car car1 = new Car();
    private Timer timer1 = new Timer(1000, new TimerListener());
    private ArrayList<Obstacle> list1 = new ArrayList<Obstacle>();
    private Random rand1 = new Random();

    public Little_car_panel() {
        setBorder(BorderFactory.createLineBorder(Color.black));
        setFocusable(true);
        addKeyListener(this);
        timer1.start();
    }



    public void keyPressed(KeyEvent e) {
        Integer key = e.getKeyCode();
        if (key == KeyEvent.VK_LEFT) {
            car1.move_left();
            check_collision();
        }
        if (key == KeyEvent.VK_RIGHT) {
            car1.move_right();
            check_collision();
        }
    }

    public void keyReleased(KeyEvent e) {

    }


    public void keyTyped(KeyEvent e) {

    }

    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        car1.draw_car(g);
        for (Obstacle obstacle : list1) {
            obstacle.draw_obstacle(g);
        }
        repaint();

    }

    public void check_collision() {
        Integer carX1 = car1.getxPos();
        Integer carX2 = carX1 + car1.getWidth();
        Integer carY1 = car1.getyPos();
        Integer carY2 = carY1 + car1.getHeight();
        for (Obstacle obstacle : list1) {
            Integer obstacleX1 = obstacle.getxPos();
            Integer obstacleX2 = obstacleX1 + obstacle.getLato();
            Integer obstacleY = obstacle.getyPos() + obstacle.getLato();
            if (((obstacleX1 >= carX1 && obstacleX1 <= carX2) || (obstacleX2 >= carX1 && obstacleX2 <= carX2))
                    && ((obstacleY >= carY1 && obstacleY <= carY2))) {
                timer1.stop();
                removeKeyListener(this);
            }

        }
    }

    public class TimerListener implements ActionListener {
        public void actionPerformed(ActionEvent e) {
            Integer x = rand1.nextInt(420);
            list1.add(new Obstacle(x));
            for (Obstacle obstacle : list1) {
                obstacle.move_down();
                check_collision();
            }
        }

    }

}

public class Car {

    private Integer xPos = 225;
    private Integer yPos = 220;
    private Integer width = 20;
    private Integer height = 40;

    public Integer getxPos() {
        return xPos;
    }

    public void setxPos(Integer xPos) {
        this.xPos = xPos;
    }

    public Integer getyPos() {
        return yPos;
    }

    public void setyPos(Integer yPos) {
        this.yPos = yPos;
    }

    public Integer getWidth() {
        return width;
    }

    public void setWidth(Integer width) {
        this.width = width;
    }

    public Integer getHeight() {
        return height;
    }

    public void setHeight(Integer height) {
        this.height = height;
    }

    public void move_right() {
        if (xPos < 420)
            xPos = xPos + width;
    }

    public void move_left() {
        xPos = xPos - width;
        if (xPos < 0) {
            xPos = 0;
        }
    }

    public void draw_car(Graphics g) {
        g.setColor(Color.RED);
        g.fillRect(xPos, yPos, width, height);
    }

}

public class Obstacle {

    private Integer xPos;
    private Integer yPos = -20;
    private Integer lato = 20;

    public Integer getxPos() {
        return xPos;
    }

    public void setxPos(Integer xPos) {
        this.xPos = xPos;
    }

    public Integer getyPos() {
        return yPos;
    }

    public void setyPos(Integer yPos) {
        this.yPos = yPos;
    }

    public Integer getLato() {
        return lato;
    }

    public void setLato(Integer lato) {
        this.lato = lato;
    }

    public Obstacle(Integer xPos) {
        super();
        this.xPos = xPos;
    }

    public void move_down() {
        yPos = yPos + lato;
    }

    public void draw_obstacle(Graphics g) {
        g.setColor(Color.BLUE);
        g.fillRect(xPos, yPos, lato, lato);
    }
}
\$\endgroup\$
  • \$\begingroup\$ You should separate these since they're different classes, as it is they all say 'public class' and there's no way that would work if it was one file. I surmise you put it altogether for the sake of this post. \$\endgroup\$ – Legato Aug 28 '15 at 12:56
4
\$\begingroup\$
  1. The Classes Car and Obstacle have the same methods and properties. This is a good reason to create a common super class with them and move xPos, yPos as well as the related method to super class.

  2. Methods draw_obstacle() and draw_car() do one logic into one place, I mean paintComponent(). The best practice is constructing special interface like IRenderable and define method paint(Graphics g) and implement it in the Car and Obstacle. This step allows your to manipulate with visible objects as one thing.

    interface IRenderable {
        void render(Graphics g);
    }
    
    // class Position as result of point 1
    class Car extends Position implement IRenderable {
    
        void render(Graphics g) {
           // place body of draw_car() 
        }
    }
    
    class Obstacle extends Position implement IRenderable {
    
        void render(Graphics g) {
           // place body of draw_obstacle() 
        }
    }
    
    public void paintComponent(Graphics g) {
        // renderable list with car and obstacles
        for (IRenderable render : renderableList) {
            render.paint(g);
        }
        repaint();
    
    }
    
\$\endgroup\$
  • \$\begingroup\$ thank you for the advice but would you mind explain point 2 in more details? \$\endgroup\$ – Alex Reds Sep 2 '15 at 14:51
  • \$\begingroup\$ I updated code block \$\endgroup\$ – mihaj Sep 3 '15 at 9:40
  • \$\begingroup\$ Thank you, can you please also make explicit how to create the renderable list? \$\endgroup\$ – Alex Reds Sep 7 '15 at 20:21
  • \$\begingroup\$ List<IRenderable> renderableList = new ArrayList<IRenderable>(); renderableList.add(yourInstanceOfCar); renderableList.add(yourInstanceOfObstacle); .... \$\endgroup\$ – mihaj Sep 8 '15 at 6:45
2
\$\begingroup\$

Follow convention standards

Conventionally Java variable and method names are camelCase, and class names are upper CamelCase, underscores are only used when representing final variables with more than one word. You can read more about conventions here. So, for example Little_Car_Frame should be LittleCarFrame

Use WindowConstants

On topic of your Little_Car_Frame instead of calling JFrame.EXIT_ON_CLOSE it should be invoking WindowConstants.EXIT_ON_CLOSE The fact JFrame still works is really due to backwards compatibility for a more sustainable solution you want to use the latter, you can read more about why here.

Consider Lambda Expressions

Since you're setting the size and doing all the work on the constructor, I think you might as well call setVisible within as well, if you do it will simplify your call in main, which I recommend using a lambda expression for, it would refactor all this:

public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    Little_car_frame frame = new Little_car_frame();
                    frame.setVisible(true);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        });
    }

to this:

public static void main(String[] args) {
   SwingUtilities.invokeLater(Little_car_frame::new);
}

This eliminates the need for EventQueue and Runnable for favor of SwingUtilities, I use your class name for portability, but again, conventionally this should be LittleCarFrame.

Don't just catch Exception

It's considered bad practice to catch Exception you want to specify exactly what you expect, though this refactoring threw none so it wasn't necessary to begin with.

More on Lambdas

Since you only use TimerListener once, you may define it anonymously and use it directly, also implementing a lambda expression, which would eliminate the need to import both ActionListener and ActionEvent

It would look like this:

private Timer timer1 = new Timer(1000, e -> {
        Integer x = rand1.nextInt(420);
        list1.add(new Obstacle(x));

        for (Obstacle obstacle : list1) {
                obstacle.move_down();
                check_collision();
        }
});

Of course, in this case, both your ArrayList and Random object need be defined prior to this or you'll get an illegal forward reference error.

Employ the diamond operator

You may use the diamond operator in your invocations. So for example, when you're initializing your obstalce array just new ArrayList<>(); is sufficient.

\$\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.