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've been working on a game so as to fill out something that I can put on a CV and I was writing the main Game class. This shouldn't be a god class, but it should simply be a class that ties together the "wiring" of the game, i.e, it should start listening for input events and should start the game loop and wire any inputs to the associated handlers.

/**
 * The main class for the Game.
 * This class should start the game loop as well as display updates.
 * @author Dan
 *
 */
public class Game implements Disposable {
    private final DisplayWindow _display;
    private final Disposable _displayUpdates;
    private final Scheduler _scheduler;

    /**
     * Create the Game.
     * @param displayService - the service to use for the Display
     * @param width - the initial width of the display
     * @param height - the initial height of the display
     */
    public Game(Scheduler gameScheduler, DisplayWindow displayWindow) {
        _display = displayWindow;
        _scheduler = gameScheduler;

        // Wait for the display to open correctly
        try {
            if(!_display.isOpen().get(500, TimeUnit.MILLISECONDS)) {
                System.err.println("failed to initialize OpenGL display!");
                System.exit(2);
            }
        } catch (Exception e) {
            System.err.println("OpenGL took too long to respond!");
            e.printStackTrace(System.err);
            System.exit(1);
        }

        // schedule display to update @ 60 fps
        int frameInterval = (int) ((1f / displayWindow.getTargetFramesPerSecond()) * 1000f);
        _displayUpdates = _scheduler.schedule(() -> displayWindow.update(), frameInterval);

    }

    public void startGameLoop() throws InterruptedException {
        while(true) {

            // Check to see if the window is closed
            // Bear in mind it's OK to wait here because
            // if it takes way too long we can assume that
            // openGL has stopped responding
            try {
                if(_display.isClosing().get(500, TimeUnit.MILLISECONDS)) {
                    break;
                }
            } catch (Exception e) {
                // Terminate
                System.err.println("OpenGL took too long to respond!");
                e.printStackTrace(System.err);
                System.exit(1);
            }
            Thread.sleep(1);
        }
    }

    public void dispose() {
        _display.dispose();
        _displayUpdates.dispose();
    }
}

Here are a few signatures you might want to know while reviewing:

  • Future<bool> _display.isOpen()
  • Future<bool> _display.isClosing()
  • Disposable _scheduler.schedule(...)

Obviously the while(true) { Thread.sleep(1); } needs some changing so it actually has a legitimate timestep.

I just wanted to get this out of the way before writing an entire application based on this class doing the functionality it does.

share|improve this question
1  
Remove e.printStackTrace(System.err); –  JaDogg Aug 24 at 18:17
    
Out of curiosity, what's wrong with it? –  Dan Pantry Aug 24 at 20:50
    
Your Game class should not assume that it owns the process. Right now, it sometimes decides to kill the process: System.exit. Only the main functions owns the process. Everyone else is a guest. –  usr Aug 24 at 22:08
    
No, that's fine, but I mean, what's wrong with e.printStackTrace :P –  Dan Pantry Aug 24 at 22:24
1  
I believe you should only use printStackTrace for development/debugging. It's considered "dirty" in production code. –  toto2 Aug 24 at 23:52

2 Answers 2

up vote 11 down vote accepted

I like to trash people when their classes do too many things. I was disappointed when I saw your class because it is not so bad.

However, I don't think this class should check if the display did open. You should only create the Game once you know that the display is ready.

I'm not sure Game is an appropriate class name.

Naming member variables starting with an underscore comes from some other language; it is not standard Java.

You write too many comments. Let the code speak for itself and you won't have to worry about keeping the code and the comments in sync.

  • The comment at the beginning at the class is clearly deprecated.
  • // schedule display to update @ 60 fps might not be true.
share|improve this answer
    
lots of deprecated comments, yeah, especially the 60fps one which I missed. I'm a .NET developer in my current job hence the prefix of member variables with _. I'll try and separate the code such that the Game instance is only created after the display is. Not sure where I would do that of course but hey :p –  Dan Pantry Aug 24 at 18:11

The responsibilities of the Game class are not clear. The name is not helping. Perhaps its responsibility is managing a game loop, in which case it would be better to call it GameLoopManager or something.

A constructor is not a good place to exit a program. A constructor is for creating an object and making sure it's complete and usable. The DisplayWindow object is a parameter of the constructor. The Game class just receives this as a parameter (from somewhere), it should not be responsible for validating it. The caller should do that.

The startGameLoop method also doesn't sound like a good place to exit the program. Its responsibility should be running the game loop. If something goes wrong, it can throw an exception and let the caller handle it appropriately.

When handling exceptions, try to avoid using the generic Exception class, use the specific Exception that might be thrown. It will be easier to understand how the code is supposed to work, and what can go wrong.

If you're "handling" an Exception is this:

e.printStackTrace(System.err);
System.exit(1);

This is little different from letting the program crash by itself. If this is how you handle (respond to) an exception, then consider adding a throws clause to your method and just let the application crash.

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.