Conceptually, when you want to perform an action at a fixed rate, you want to use an ScheduledExecutorService. This class is designed to handle the use-case of running code at a fixed rate, possibly with an initial delay. The advantage is that you can schedule more than one task, and it also lets you handle all the tasks, eventually cancelling some of them. It's not so much that the while(true) is an issue per se, but doing all those operations with that approach would be a lot more complicated. Furthermore, it wouldn't really let you perform other unrelated actions at the same time as the code getting the badges. Additionally, don't use the Timer class, and always prefer a ScheduleExecutorService, when you can use one.
Rewriting the code to use the service would actually make it more clear. First of all, you want a method that represents the action to perform:
private static void printBadges() {
JsonArray weeds;
try {
weeds = getBadges().get("items").getAsJsonArray();
} catch (IOException e) {
e.printStackTrace(); // this should be replaced with a real logger
return;
}
int numberOfWeeds = weeds.size();
if(numberOfWeeds!=0) {
System.out.println("[ [Badger](https://www.youtube.com/watch?v=dQw4w9WgXcQ) ] " + numberOfWeeds + " new [Tumbleweed posts](//stackoverflow.com/help/badges/63/tumbleweed)");
}
previousBadgeTimestamp = Instant.now();
}
With that method in place, scheduling the task is as easy as
ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
executorService.scheduleAtFixedRate(RunBadger::printBadges, 0, 10, TimeUnit.MINUTES);
This documents clearly what is happening, mainly that something scheduled at a fixed rate, and the delay is obvious: 10 minutes. The second parameter is a potential initial delay before performing the action the first time, which I set to 0 here.
A note about the exception handling. The current code catches the exception outside of the while loop, which means that any exception will stop the program from running; generally, this isn't want you want. An exception from getting the badges could occurs for multiple reasons and it shouldn't stop the rest of the code from running (although in this case, there is no other part of the code, but imagine this in a more complex scenario, with other tasks running concurrently). The scheduledAtFixedRate method will suppress the execution of future tasks, thereby cancelling the schedule, if there is an exception thrown by the task: this means that if an exception is thrown, nothing will get executed anymore. Catching and handling the exceptions inside the task, like done in the example above, lets you manage this.
The question of what should happen after scheduling the task remains. If you just want to run it for fun until you kill the process manually, you can just keep this in the main method:
public static void main(String[] args) {
ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
executorService.scheduleAtFixedRate(Main::printBadges, 0, 1, TimeUnit.SECONDS);
}
The executor service won't be garbage collected and the action will run every 10 minutes, until you kill the JVM yourself. A cleaner approach would be to invoke executorService.shutdown(); when you want the process to stop, as it will correctly stop the executor, and let the main thread finish properly. (Note that it won't block until the tasks are finished; awaitTermination can be used for that). You just need to find the stopping condition, like a specific exception thrown in the task. It could be added as a runtime hook as well, but note that those hooks don't run when killing with SIGKILL.
Other thoughts:
JsonObject root = new JsonParser().parse(json).getAsJsonObject();
return root;
You don't need to store this in a temporary variable just before returning. Returning directly is just as clear, and doesn't introduce a variable.
return new JsonParser().parse(json).getAsJsonObject();
public static JsonObject getBadges() throws IOException
This may tie a bit the code fetching the badges to the code printing the result. In fact, you're only interested in the number of new items, so make that method return the count directly:
public static int getBadgesCount() throws IOException
This way, the rest of the code is hidden from the internal JSON representation of what the API returns.
public static Instant previousBadgeTimestamp
That's very good use of Instant: it represents a point in the timeline in UTC and isn't tied to any specific timezone. When you want to model at what time a system event took place, Instant is the perfect class.
private static final String apiKey = "my api key";
If the API key deserves its constant, why not all the other hard-coded Strings like badgeIdUrl or the rick-rolling very interesting link badger links to? You could even make all of those properties dynamic, by reading them from a file.
if(numberOfWeeds!=0) {
Since this number will never be negative, it is clearer to write
if(numberOfWeeds > 0) {
as it doesn't let the reader wonder whether accepting a negative value was intended or not.
Final point: keep in mind that the StackExchange API has a rate limit in place, and if the API returns a backoff, you have to wait that amount of seconds before making another call.
