5
\$\begingroup\$

Today I started to build my first platformer, first time working with collisions and stuff. Now, surprisingly, everything seems to work, but since this is my first time creating a platformer and I'm still a beginner with collisions I don't know if the way I coded it is good practise.

The game is a ball with a fixed x coordinate and it has to jump to different platforms that move from the right to the left. If the ball falls through the bottom or gets too high, i.e. not being visible anymore, the ball freezes. To see if the ball has fallen through the bottom of the screen I used the variable HEIGHT (declared in class GamePanel), but instead of the ball freezing outside of the screen, you can still see a little bit of it on the screen when it has frozen.

If anything in the code is unclear, please tell me, because I want to learn to write readable code. Did I use enough comments? Too many? On the wrong spots?

BasicPlatformer.java

package BasicPlatformer;

import javax.swing.*;

public class BasicPlatformer {

    public static void main(String[] args){
        GamePanel gamePanel = new GamePanel();
        JFrame frame = new JFrame("Basic Platforrmer");
        frame.getContentPane().add(gamePanel);
        frame.pack();
        frame.setVisible(true);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setLocation(2000, 50);
        frame.setResizable(false);
        gamePanel.requestFocusInWindow();
    }
}

GamePanel.java

package BasicPlatformer;

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.util.ArrayList;    

public class GamePanel extends JPanel {
    public static final int WIDTH = 1000;
    public static final int HEIGHT = 900;
    public static final int PLATFORMSTARTHEIGHT = 700;

    public static ArrayList<Platform> platforms;

    Timer platformTimer, playerTimer, creationTimer;
    Player player;

    public GamePanel(){
        Dimension panelSize = new Dimension(WIDTH, HEIGHT);
        setPreferredSize(panelSize);
        setBackground(Color.BLACK);
        setFocusable(true);
        addKeyListener(new KeyInputListener());
        platformTimer = new Timer(5, new PlatformAnimationListener());
        playerTimer = new Timer(35, new PlayerAnimationListener());
        creationTimer = new Timer(3600, new PlatformCreationListener());
        startGame();
    }

    class KeyInputListener extends KeyAdapter {
        @Override
        public void keyPressed(KeyEvent e){
            if(e.getKeyCode() == 32) //space bar
                player.jump();
        }
    }

    public void startGame(){
        platforms = new ArrayList<>();
        platforms.add(new Platform(0, 1000)); //the 1000 pixel wide starting platform
        player = new Player();

        platformTimer.start();
        playerTimer.start();
        creationTimer.setInitialDelay(1700);
        creationTimer.start();
    }

    class PlatformAnimationListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent e){
            for(int i = 0; i < platforms.size(); i++){
                if(platforms.get(i).getX() < 0 - platforms.get(i).getW()) //remove platforms once they aren't visible
                    platforms.remove(platforms.get(i));
                platforms.get(i).move();
            }
            repaint();
        }
    }

    class PlayerAnimationListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent e){
            player.move();
            repaint();
        }
    }

    class PlatformCreationListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent e){
            Platform platform = new Platform();
            platforms.add(platform);
        }
    }

    public void paintComponent(Graphics g){
        super.paintComponent(g);
        for(Platform platform : platforms)
            platform.display(g);
        player.display(g);

        if(!player.isAlive){
            drawGameOver(g);
        }
    }

    public void drawGameOver(Graphics g){
        Graphics2D g2d = (Graphics2D) g.create();
        g2d.setColor(Color.WHITE);
        Font gameOverFont = new Font("Arial", Font.BOLD, 80);
        g2d.setFont(gameOverFont);
        String gameOverString = "Game Over";
        drawString(gameOverString, g2d);
        g2d.dispose();
    }

    public void drawString(String text, Graphics2D g2d){
        FontMetrics fm = g2d.getFontMetrics();
        int x = (WIDTH - fm.stringWidth(text)) / 2;
        int y = (fm.getAscent() + (HEIGHT - (fm.getAscent() + fm.getDescent())) / 3);
        g2d.drawString(text, x, y);
    }
}

Platform.java

package BasicPlatformer;

import java.awt.*;
import java.util.Random;

public class Platform {

    private int x = 1000;
    private int y = GamePanel.PLATFORMSTARTHEIGHT;
    private int w = 300;
    private int h = 25;
    private static int prevY = GamePanel.PLATFORMSTARTHEIGHT;

    public Platform(){
        do {
            int maxDif = 150; //maximum height difference between the current and the next platform
            int random = new Random().nextInt(maxDif * 2) - maxDif; //gets a random number between -maxDif and maxDif
            y = prevY + random;
        } while (y < 0 || y > GamePanel.HEIGHT - 200); //if y is lower than the bottom or higher than 200 pixels from the top, try again
        prevY = y;
    }

    public Platform(int startX, int startW){
        x = startX;
        w = startW;
    }

    public int getX(){
        return x;
    }

    public int getY(){
        return y;
    }

    public int getW(){
        return w;
    }

    public void move(){
        x--;
    }

    public void display(Graphics g){
        Graphics2D g2d = (Graphics2D) g.create();
        g2d.setColor(Color.DARK_GRAY);
        g2d.fillRect(x, y, w, h);

        g2d.dispose();
    }
}

Player.java

package BasicPlatformer;

import java.awt.*;

public class Player {

    private int w = 50;
    private int h = w;
    private int x = 100;
    private int y = GamePanel.PLATFORMSTARTHEIGHT - h;
    private int velY;
    public boolean isAlive;

    public Player(){
        isAlive = true;
    }

    public void jump(){
        velY = -20;
    }

    public void move(){
        if(isAlive) {
            y += velY;
            fall();
            checkCollision();
        }
    }

    public void checkCollision(){
        int bX = x + w / 2; //x of the middle bottom of the player
        int bY = y + h; //y of the bottom of the player

        for (Platform platform : GamePanel.platforms) {
            if (bX > platform.getX() && bX <= platform.getX() + platform.getW()) { //check if there's any platform under or above the player
                if (bY >= platform.getY() && bY < platform.getY() + velY) { //because velY is added to y, some y values are skipped. But a y value of the player must be between the y of the platform and the value of velY below the y of the platform
                    velY = 0;
                    y = platform.getY() - h; //because the player can land a bit off of the platform, set the y of the player to the y of the platform and ajust with the height of the player
                }
                break;
            }
        }
        checkAlive(bY);
    }

    public void fall(){
        velY++;
    }

    public void checkAlive(int bY){
        if (y > GamePanel.HEIGHT || bY < 0) {
            isAlive = false;
        }
    }

    public void display(Graphics g){
        Graphics2D g2d = (Graphics2D) g.create();
        g2d.setColor(Color.GRAY);
        g2d.fillOval(x, y, w, h);
        g2d.dispose();
    }
}
\$\endgroup\$
5
\$\begingroup\$
  1. All GUI updates must be performed on the GUI thread. But you create new GUI elements in the main method of the BasicPlatformer, which runs on the main thread. It is not correct. You should use the invokeLater method and a Runnable(possibly with a lambda expression) instead.

  2. It looks like there is no need for multiple timers here. One timer(with the shortest delay) can do fine(it just requires small changes in the event listener).

  3. Overall design: the Player class knows to much about the internals of the GamePlatform and the Platform. It seems to me that intersection methods should belong to the Platform class and bounds checks should be in the GamePlatform class.
    You code also violates the single responsibility principle: the game logic is tightly coupled with the GUI. I'd recommend creating separate classes for performing computations and rendering the game.

  4. Variables names. There is no need to make them so short. Keep them descriptive. For example, this piece of code:

    int bX = x + w / 2; //x of the middle bottom of the player
    int bY = y + h; //y of the bottom of the player
    

    is not good. It requires writing additional comments instead of storing all the information in the code itself:

    int playerBottomX = x + w / 2; 
    int playerBottomY = y + h; 
    

    Now it is clear that these are the coordinates of the bottom of the player without any comments(it would also be good to change x and y to something more descriptive: what part of the player do they represent? Why do we add w / 2 to x? Ideally, the code should be written in such a way that answers to these questions are obvious).

    One more example of an unclear method:

    for (Platform platform : GamePanel.platforms) {
        if (bX > platform.getX() && bX <= platform.getX() + platform.getW()) { //check if there's any platform under or above the player
            if (bY >= platform.getY() && bY < platform.getY() + velY) { //because velY is added to y, some y values are skipped. But a y value of the player must be between the y of the platform and the value of velY below the y of the platform
                velY = 0;
                y = platform.getY() - h; //because the player can land a bit off of the platform, set the y of the player to the y of the platform and ajust with the height of the player
            }
            break;
        }
    }
    

    There is too much stuff going on here. And the fact that you need to write comments to explain what the code does is a smell. How to improve it? Well, you can make it much more readable by decomposing it into several smaller methods:

    for (Platform platform : GamePanel.platforms) {
        if (platform.isAboveHorizontally(playerBottomX)) { 
            if (platform.intersectsVertically(playerBottomY, velocityY)) { 
                landOnPlatform(platform);
            }
            break;
        }
    }
    

    I think that intersection methods should belong to the Platform class.

    A rule of thumb: if you need to write comments inside a method to clarify what's going on, you can either rename variables or extract a method instead. The code should be self-documenting.

  5. As I have said above, it is often(not always) a smell to have comments inside a method. However, it is a good practice to write comments for all public classes and methods(which explain what they do, not how).

  6. Do not make fields static unless it is really necessary. For instance, the platforms field of the GamePanel class does not look like a good candidate to be static. It belongs to an instance, not to a class(what happens if you create more than instance?).

  7. Avoid "generic" names for your methods. For example, move does not tell much about what the method does. The questions is: move where? The same is true for fields.

  8. Use consistent indentation: always have one whitespace before an opening curly bracket.

  9. Now, surprisingly, everything seems to work

    It looks like you are not confident in your code. It is a good practice to test the code in a systematic manner(using unit tests) to be sure that it works(of course, tests cannot prove its correctness, but they can help to ensure that it behaves properly for specific cases. It is much better than nothing).

\$\endgroup\$
  • \$\begingroup\$ Thanks, I've learned a lot from this post. But do you know why the game says you're game over a little before the player is actually off the screen? (Also asked this above) Again thanks for the long and good answer \$\endgroup\$ – The Coding Wombat May 15 '15 at 21:37

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.