Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

This idea is cool, I have some suggestions, though.

First off, you definitely should add JavaDoc to your annotation.

I wouldn't know that fps is preferred over delay, if you hadn't written it in your question (well I could find out, but it'd be a hassle). This should be done by documentation.
You can also document default values and usage constraints there, which brings me to the next point:

Your annotation only works if you annotate the run method. You might want to consider changing that a little and grab the method depending on the annotation you have (and not check for the annotation on your run method).
But that's mostly up to you ;)

Next thing I want to talk about is the units you're using.

Your delay is an int. I strongly assume the delay is given in milliseconds. If you'd want to reuse this it might be interesting to allow specifying the delay with an arbitrary TimeUnit, but that's future talk.

What's more interesting is things like this code:

int timePerFrame = 1000 / fps;

time as what? be explicit and tell the reader exactly what you mean and say: msPerFrame

It's somewhat similar when you grab the sleepTime. When first reading this I was somewhat.. confused. Tell the reader you need to divide by 1 mil. because you need to get from nanoseconds to milliseconds.

##BUG ALERT

BUG ALERT

Btw, that's a bug right there. Counting the zeroes I'm getting to 10 mil. instead of 1. To prevent this you can write numeric literals with underscores in between them for easier overview:

long sleepTime = (System.nanoTime() - start) / 1_000_000 + msPerFrame;

BUG EXTERMINATION COMPLETE

Moving swiftly on to how you handle delay. It's very interesting to see the difference in how you solved the runtime of run problem from fps to delay.

I'd have expected you to do the same thing as with the fps. Instead you wait for run to complete before counting down the delay. If you want to have the same handling so fps and delay are usable interchangeably, you should do the following:

while (engine.isRunning()) {
 long start = System.nanoTime();
 run();
 long remainingDelay = delay - ((System.nanoTime() - start) / 1_000_000);
 
 if (remainingDelay > 0) {
 Thread.sleep(remainingDelay);
 } else {
 Thread.yield();
 }
}

which actually was another (rather minor) bug.

Movig swiftly on to your AppEngine, where I see you do the following twice:

running.getAndSet(...);

Which makes me ask myself: Why not just use set() or lazySet()? It seems you used to return booleans, eh? Oh and another thing:

If you already make all the methods you have final, why not also make the class final? Then you can even remove the final from all the methods you have ;)

This idea is cool, I have some suggestions, though.

First off, you definitely should add JavaDoc to your annotation.

I wouldn't know that fps is preferred over delay, if you hadn't written it in your question (well I could find out, but it'd be a hassle). This should be done by documentation.
You can also document default values and usage constraints there, which brings me to the next point:

Your annotation only works if you annotate the run method. You might want to consider changing that a little and grab the method depending on the annotation you have (and not check for the annotation on your run method).
But that's mostly up to you ;)

Next thing I want to talk about is the units you're using.

Your delay is an int. I strongly assume the delay is given in milliseconds. If you'd want to reuse this it might be interesting to allow specifying the delay with an arbitrary TimeUnit, but that's future talk.

What's more interesting is things like this code:

int timePerFrame = 1000 / fps;

time as what? be explicit and tell the reader exactly what you mean and say: msPerFrame

It's somewhat similar when you grab the sleepTime. When first reading this I was somewhat.. confused. Tell the reader you need to divide by 1 mil. because you need to get from nanoseconds to milliseconds.

##BUG ALERT

Btw, that's a bug right there. Counting the zeroes I'm getting to 10 mil. instead of 1. To prevent this you can write numeric literals with underscores in between them for easier overview:

long sleepTime = (System.nanoTime() - start) / 1_000_000 + msPerFrame;

BUG EXTERMINATION COMPLETE

Moving swiftly on to how you handle delay. It's very interesting to see the difference in how you solved the runtime of run problem from fps to delay.

I'd have expected you to do the same thing as with the fps. Instead you wait for run to complete before counting down the delay. If you want to have the same handling so fps and delay are usable interchangeably, you should do the following:

while (engine.isRunning()) {
 long start = System.nanoTime();
 run();
 long remainingDelay = delay - ((System.nanoTime() - start) / 1_000_000);
 
 if (remainingDelay > 0) {
 Thread.sleep(remainingDelay);
 } else {
 Thread.yield();
 }
}

which actually was another (rather minor) bug.

Movig swiftly on to your AppEngine, where I see you do the following twice:

running.getAndSet(...);

Which makes me ask myself: Why not just use set() or lazySet()? It seems you used to return booleans, eh? Oh and another thing:

If you already make all the methods you have final, why not also make the class final? Then you can even remove the final from all the methods you have ;)

This idea is cool, I have some suggestions, though.

First off, you definitely should add JavaDoc to your annotation.

I wouldn't know that fps is preferred over delay, if you hadn't written it in your question (well I could find out, but it'd be a hassle). This should be done by documentation.
You can also document default values and usage constraints there, which brings me to the next point:

Your annotation only works if you annotate the run method. You might want to consider changing that a little and grab the method depending on the annotation you have (and not check for the annotation on your run method).
But that's mostly up to you ;)

Next thing I want to talk about is the units you're using.

Your delay is an int. I strongly assume the delay is given in milliseconds. If you'd want to reuse this it might be interesting to allow specifying the delay with an arbitrary TimeUnit, but that's future talk.

What's more interesting is things like this code:

int timePerFrame = 1000 / fps;

time as what? be explicit and tell the reader exactly what you mean and say: msPerFrame

It's somewhat similar when you grab the sleepTime. When first reading this I was somewhat.. confused. Tell the reader you need to divide by 1 mil. because you need to get from nanoseconds to milliseconds.

BUG ALERT

Btw, that's a bug right there. Counting the zeroes I'm getting to 10 mil. instead of 1. To prevent this you can write numeric literals with underscores in between them for easier overview:

long sleepTime = (System.nanoTime() - start) / 1_000_000 + msPerFrame;

BUG EXTERMINATION COMPLETE

Moving swiftly on to how you handle delay. It's very interesting to see the difference in how you solved the runtime of run problem from fps to delay.

I'd have expected you to do the same thing as with the fps. Instead you wait for run to complete before counting down the delay. If you want to have the same handling so fps and delay are usable interchangeably, you should do the following:

while (engine.isRunning()) {
 long start = System.nanoTime();
 run();
 long remainingDelay = delay - ((System.nanoTime() - start) / 1_000_000);
 
 if (remainingDelay > 0) {
 Thread.sleep(remainingDelay);
 } else {
 Thread.yield();
 }
}

which actually was another (rather minor) bug.

Movig swiftly on to your AppEngine, where I see you do the following twice:

running.getAndSet(...);

Which makes me ask myself: Why not just use set() or lazySet()? It seems you used to return booleans, eh? Oh and another thing:

If you already make all the methods you have final, why not also make the class final? Then you can even remove the final from all the methods you have ;)

Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

This idea is cool, I have some suggestions, though.

First off, you definitely should add JavaDoc to your annotation.

I wouldn't know that fps is preferred over delay, if you hadn't written it in your question (well I could find out, but it'd be a hassle). This should be done by documentation.
You can also document default values and usage constraints there, which brings me to the next point:

Your annotation only works if you annotate the run method. You might want to consider changing that a little and grab the method depending on the annotation you have (and not check for the annotation on your run method).
But that's mostly up to you ;)

Next thing I want to talk about is the units you're using.

Your delay is an int. I strongly assume the delay is given in milliseconds. If you'd want to reuse this it might be interesting to allow specifying the delay with an arbitrary TimeUnit, but that's future talk.

What's more interesting is things like this code:

int timePerFrame = 1000 / fps;

time as what? be explicit and tell the reader exactly what you mean and say: msPerFrame

It's somewhat similar when you grab the sleepTime. When first reading this I was somewhat.. confused. Tell the reader you need to divide by 1 mil. because you need to get from nanoseconds to milliseconds.

##BUG ALERT

Btw, that's a bug right there. Counting the zeroes I'm getting to 10 mil. instead of 1. To prevent this you can write numeric literals with underscores in between them for easier overview:

long sleepTime = (System.nanoTime() - start) / 1_000_000 + msPerFrame;

BUG EXTERMINATION COMPLETE

Moving swiftly on to how you handle delay. It's very interesting to see the difference in how you solved the runtime of run problem from fps to delay.

I'd have expected you to do the same thing as with the fps. Instead you wait for run to complete before counting down the delay. If you want to have the same handling so fps and delay are usable interchangeably, you should do the following:

while (engine.isRunning()) {
 long start = System.nanoTime();
 run();
 long remainingDelay = delay - ((System.nanoTime() - start) / 1_000_000);
 
 if (remainingDelay > 0) {
 Thread.sleep(remainingDelay);
 } else {
 Thread.yield();
 }
}

which actually was another (rather minor) bug.

Movig swiftly on to your AppEngine, where I see you do the following twice:

running.getAndSet(...);

Which makes me ask myself: Why not just use set() or lazySet()? It seems you used to return booleans, eh? Oh and another thing:

If you already make all the methods you have final, why not also make the class final? Then you can even remove the final from all the methods you have ;)

lang-java

AltStyle によって変換されたページ (->オリジナル) /