Problem
I have been working on a small game lately and I was tired of looking up and copy-pasting game loops so I tried to make one as you can see. It works perfectly so if you want to feel free to use it, but for those who are good at making these I would like to ask if I can improve it, make it more efficient and overall is it a good solution to run two of these one to render, the other to tick.
package engine;
public abstract class RapidExecutor extends Thread {
Application app;
/*
the field app is here because the abstract tick method can
only acces the stuff needed to render or to tick
objects so if you want to use it for anything else
you can just delete it
*/
private final int TPS;
private boolean running;
public RapidExecutor(Application app, int TPS) {
this.app = app;
this.TPS = TPS;
if (TPS > 1000 || TPS < 1)
try {
throw new Exception("Invalid TPS count");
} catch (Exception e) {
e.printStackTrace();
}
}
public abstract void tick();
public void run() {
try {
this.running = true;
long now, startTime;
int delta, TPSCount = 1000 / this.TPS;
while (this.running) {
startTime = System.currentTimeMillis();
tick();
now = System.currentTimeMillis();
delta = (int) (TPSCount - (now - startTime));
if (delta < 0) this.stopThread();
sleep(delta);
}
} catch (Exception e) {
e.printStackTrace();
}
}
public synchronized void stopThread() {
try {
this.running = false;
this.join();
} catch (Exception e) {
e.printStackTrace();
}
}
}
Feel free to use the code and if you have any thoughts I would be very glad to hear them.
Solution
Before reviewing the code, I want to point out that this kind of functionality already exists in the core Java libraries. A ScheduledExecutorService will run an arbitrary Runnable on a fixed time schedule. Instead of extending RapidExecutor, you just implement the interface and pass it into the service.
Some thoughts on your code:
In my opinion, your code is violating the Single Responsibility Principle. It’s doing three things: defining the interface for something that executes every tick, defining a new type of Thread, and controlling the frequency that this new type of thread executes code. As mentioned above, I’d strongly suggest using ScheduledExecutorService, Thread, and implement Runnable for the code that is currently in subclasses of RapidExecutor.
Your Application app should be private, tick() should be tick(Application app), and run() should call tick(this.app). Keep variables contained in scope.
Don’t use UPPERCASE for abbreviations. It’s considered better to use camelCase (tps or tpsCount). And don’t abbreviate when you don’t need to. It took me a bit to figure out that TPS was ticksPerSecond.
Please don’t throw an exception just to catch it to get the stack trace to print it out. Let the caller figure out what to do when you tell them they gave you bad input. They might not care about the error message being written to System.err, or they might actively not want that to happen. And don’t use a generic Exception. IllegalArgumentException, thrown from the constructor, would be better.
tick() should be protected, not public. You only want it called from inside RapidExecutor.
run() must be final. If it isn’t, anybody who extends RapidExecutor can change what it means for run() to be executed.
It’s considered better form to declare variables on separate lines.
Variables should be declared in the tightest scope possible to reduce cognitive load on readers of your code. startTime, now, and delta should all be declared inside your while loop.
There’s no need to cast delta back to an int. And perhaps sleepTime would be a better variable name than delta? It’s good for variables to clearly express what the represent.
Perhaps storing final long tickDuration = System.currentTimeMillis() - startTime would be better than now.
Perhaps renaming TPSCount to millisecondsPerTick would make it more clear what was being stored.
Again, don’t randomly catch Exception. And if you do handle an exception, reset the state of RapidExecutor to something appropriate. Right now execution stops but this.running is still true. Are you trying to handle the InterruptedException? If so, declare that try-catch block immediately around sleep(). There are well-understood ways to correctly handle InterruptedException, and you should look into them. Are you trying to handle exceptions coming from tick()? If so, put a tight block around this.tick(). Maybe even declaring your own checked exception in this case would be a good idea.
It’s unclear why you stop execution when it takes longer than TPSCount to run a tick(). I’m not convinced that’s a good design. Would it be better to skip a tick and get a choppy frame rate rather than just failing silently? Make sure that this is clearly documented.
Don’t ever synchronize a method using the synchronized keyword. Locking on yourself is never a good idea because somebody else could take out a lock on you, and then stopThread() will never be able to execute. Use a private Object lock along with synchronized(this.lock), or a more complex approach when warranted.
I also don’t see why you need to synchronize either of the method calls inside stopThread(). Are you sure that’s necessary?
You’ll need to make stopThread() final to prevent subclasses from changing what it means to stop a thread.
Again, make the exception block in stopThread() as tight as possible and handle the most specific exception possible.
If you applied all my suggestions to your code, it might look something like:
public abstract class RapidExecutor extends Thread {
private final Object lock = new Object();
private final Application application;
private final int ticksPerSecond;
private boolean running;
public RapidExecutor(final Application application, final int ticksPerSecond) {
this.application = application;
this.ticksPerSecond = ticksPerSecond;
if (ticksPerSecond > 1000 || ticksPerSecond < 1) {
throw new IllegalArgumentException("ticksPerSecond must be between 1 and 1000, was " + ticksPerSecond);
}
}
protected abstract void tick(final Application application) throws TickFailedException;
@Override
public void run() {
this.running = true;
final int millisecondsPerTick = 1000 / this.ticksPerSecond;
while (this.running) {
final long startTime = System.currentTimeMillis();
try {
this.tick(this.application);
} catch (final TickFailedException | RuntimeException e) {
this.running = false;
// log the exception ?
}
final long tickDuration = System.currentTimeMillis() - startTime;
final long sleepTime = millisecondsPerTick - tickDuration;
if (sleepTime < 0) {
this.stopThread();
}
try {
sleep(sleepTime);
} catch (final InterruptedException e) {
this.running = false;
this.interrupt();
// log the exception?
}
}
}
public final void stopThread() {
synchronized (this.lock) {
this.running = false;
try {
this.join();
} catch (final InterruptedException e) {
this.interrupt();
e.printStackTrace();
}
}
}
}