Does my Game class have too many responsibilities?

Question

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.

Answer 1

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.

Answer 2

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.