Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a game loop for a game I'm going to be making:

  1. Check the current FPS
  2. Try the keep the FPS to a hardcoded level
  3. Draw and Update the game.

There are two classes, the abstract class and the implementing class.

Any suggestions?

Game

public abstract class Game {
    private int FRAMES_PER_SECOND;
    private boolean running = true;
    long targetTime;

    private long runningFPS;

    protected Game(int fps) {
        setTargetFPS(fps);
    }

    public void setTargetFPS(int fps) {
        this.FRAMES_PER_SECOND = fps;
        targetTime = 1000 / FRAMES_PER_SECOND;
    }

    public void run(JPanel panel, BufferedImage image) {
        int currentFPS = 0;
        long counterstart = System.nanoTime();
        long counterelapsed = 0;
        long start;
        long elapsed;
        long wait;
        targetTime = 1000 / FRAMES_PER_SECOND;

        while (running) {
            start = System.nanoTime();

            processInput();
            update();
            // time to update and process input
            elapsed = System.nanoTime() - start; 
            wait = targetTime - elapsed / 1000000;
            if (hasTimeToDraw(wait)) {
                //CREATE AND ANTIALIAS GRAPHICS
                Graphics2D g = image.createGraphics();
                g.addRenderingHints(new RenderingHints(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON));
                g.setRenderingHint(RenderingHints.KEY_TEXT_ANTIALIASING, RenderingHints.VALUE_TEXT_ANTIALIAS_ON);
                g.setRenderingHint(RenderingHints.KEY_RENDERING, RenderingHints.VALUE_RENDER_QUALITY);
                //Draw
                draw(g);
                g.dispose();
                panel.repaint();
                //Take account for the time it took to draw
                elapsed = System.nanoTime() - start;
                wait = targetTime - elapsed / 1000000;
            }
            counterelapsed = System.nanoTime() - counterstart;
            currentFPS++;

            // at the end of every second
            if (counterelapsed >= 1000000000L) {
                //runningFPS is how many frames we processed last second
                runningFPS = currentFPS;
                currentFPS = 0;
                counterstart = System.nanoTime();
            }

            //dont wanna wait for negative time
            if (wait < 0)
                wait = 0;
            try {
                Thread.sleep(wait);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    public long getCurrentFPS() {
        return runningFPS;
    }

    private boolean hasTimeToDraw(long wait) {
        //Not really sure how to implement this method... Maybe just time the draw method and hardcode it in?
        return true;
    }

    public void stop() {
        running = false;
    }

    public abstract void processInput();

    public abstract void update();

    public abstract void draw(Graphics2D g);
}

GameFrame

public class GameFrame extends JFrame{

    private static final long serialVersionUID = 1L;
    private static final int WIDTH = 800;
    private static final int HEIGHT = 800;

    private JPanel panel;
    private BufferedImage image = new BufferedImage(WIDTH, HEIGHT, BufferedImage.TYPE_3BYTE_BGR);
    private Game game = new Game(60){
        int i = 0;
        int x = 5;

        @Override
        public void processInput() {
            // TODO Auto-generated method stub

        }
        @Override
        public void update() {
            GameFrame.this.setTitle("FPS: " + getCurrentFPS());
            if(i > WIDTH || i < 0)
                x = -x;

            i += x;
        }
        @Override
        public void draw(Graphics2D g) {
            g.setColor(Color.BLACK);
            g.fillRect(0, 0, WIDTH, HEIGHT);

            g.setColor(Color.RED);
            g.fillRect(i, 50, 20, 53);
        }

    };

    public GameFrame(){
        panel = new JPanel(){
            private static final long serialVersionUID = 1L;

            @Override
            protected void paintComponent(Graphics g) {
                g.drawImage(image, 0, 0, null);
            }
        };
        panel.setPreferredSize(new Dimension(WIDTH,HEIGHT));
        this.add(panel);
        this.pack();
        this.setVisible(true);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    public void run(){
        game.run(panel,image);
    }

    public static void main(String[] args){
        new GameFrame().run();
    }
}
share|improve this question

2 Answers 2

up vote 6 down vote accepted
long start;
long elapsed;
long wait;

This is no Pascal, declare your variable where they get initialized. This is not always possible, maybe 1 such variable per 10 classes is unavoidable.

        //dont wanna wait for negative time
        if (wait < 0)
            wait = 0;
        try {
            Thread.sleep(wait);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

Better don't change a variable if you don't have to as it makes the code harder to understand. Doing

Thread.sleep(wait<0 ? 0 : wait);

is simpler, doing

Thread.sleep(Math.max(0, wait));

is even simpler and so is

if (wait>0) Thread.sleep(wait);

The main problem is that your method is far too long. Extracting some methods would help a lot, e.g.,

  • Graphics2D g = newNiceGraphics(image);
  • sleepUninterruptibly(wait);

Enough for now....

share|improve this answer

Reserve all capital names to static final constants. It seems that a better name instead of FRAMES_PER_SECOND would be targetFPS.

Also, this variable can be final, because you assign it once, though this is not clear immediately. It becomes clear when you reorganize the constructor and get rid of the pointless setTargetFPS method, like this:

private final int targetFPS;

protected Game(int fps) {
    this.targetFPS = fps;
}

targetTime doesn't need to be a member variable: although you set it in the constructor, you don't use that initial value, and then you overwrite it in the run method. So change that from a member variable to a local variable in the run method.

In the run method, if you don't want to wait for negative time, then don't! Instead of overwriting the value of wait, use conditionals. It's also pointless to wait for 0 time:

if (wait > 0) {
    try {
        Thread.sleep(wait);
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

In GameFrame, the image and panel fields can be final. In general, make member fields final if possible. Non-editable things are inherently robust and lead to safer code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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