I have written a small program to query the Stack Exchange API, specifically for badges. The main idea of the program is to return the new list of Tumbleweed posts on Stack Overflow, every 10 minutes. The motivation for this bot was the Weed Eater hat.

The program queries the API every 10 mins. If there are new tumbleweed posts, Then it'll print the number of new posts with a link to the tumbleweed page. If there are no new ones, It waits for another 10 minutes.

The code for the same is

import com.google.gson.JsonArray;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import org.jsoup.Connection;
import org.jsoup.Jsoup;
import java.io.IOException;
import java.time.Instant;

public class RunBadger {

    private static final String apiKey = "my api key";
    public static Instant previousBadgeTimestamp = Instant.now();
    public static void main(String[] args) {
        try {
            while (true) {
                JsonArray weeds = getBadges().get("items").getAsJsonArray();
                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();
                Thread.sleep(600000);
            }
        }
        catch (Exception e){
            e.printStackTrace();
        }

    }
    public static JsonObject getBadges() throws IOException{
        String badgeIdUrl = "https://api.stackexchange.com/2.2/badges/63/recipients";
        JsonObject badgeJson = get(badgeIdUrl,"site","stackoverflow","pagesize","100","fromdate",String.valueOf(previousBadgeTimestamp.minusSeconds(1).getEpochSecond()),"key",apiKey);
        return badgeJson;
    }
    public static JsonObject get(String url, String... data) throws IOException {
        Connection.Response response = Jsoup.connect(url).data(data).method(Connection.Method.GET).ignoreContentType(true).ignoreHttpErrors(true).execute();
        String json = response.body();
        if (response.statusCode() != 200) {
            throw new IOException("HTTP " + response.statusCode() + " fetching URL " + (url) + ". Body is: " + response.body());
        }
        JsonObject root = new JsonParser().parse(json).getAsJsonObject();
        return root;
    }
}

I'd like to have an improved version of it. Particularly regarding the while(true) code block.

  • I'm new to Codereview, Do edit my post if there are any issues. Also, the APIKey is not a secret. Hence, If there is a need of that, do ask for it. – Bhargav Rao Nov 30 '16 at 16:16
  • 1
    You can edit your post to fix issues up to the point when you get an answer, at which point edits usually need to stop to prevent answer invalidation. See meta.codereview.stackexchange.com/a/1765/4203 for what you can do after getting an answer. – forsvarir Nov 30 '16 at 16:42
  • @forsvarir Thanks for the link. I went through that now. – Bhargav Rao Nov 30 '16 at 16:55
  • Small Note: I've utilized the suggestions given here and created the Badger Bot. The code is on github and the bot is live in the SOBotics chat room. – Bhargav Rao Dec 6 '16 at 12:37
up vote 12 down vote accepted

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.

  • 2
    I got up in the middle of the night just to read your answer. It's wonderful. Thanks. – Bhargav Rao Nov 30 '16 at 21:14

I don't do a lot of Java, so take this for what it is: grain of salt.


Don't use a while (true) loop, they're rarely every necessary. Manually sleeping isn't necessary either. You should use a Timer instead.

I'm not sure if it will be better across platforms (not a Java expert), but I could see how in some cases your code would cause unpleasantries with the sleeping and whatnot. What happens when you port to Android (et. al.) or something and Android sees the app as "not responding" because it's not handling any requests from the OS? We have this same problem in .NET (especially with Windows Forms) where the application hangs due to not handling Windows message.


I don't know what your use-case for this is, but swallowing the exception outside the while loop like that is probably bad in some cases. Consider a case when the API temporarily throws an error for that endpoint.

Instead, I would consider a 500 error message differently from all other error messages. In fact, I would expect that you should consider three different situations:

  1. 200 success message, which you have;
  2. 500 API error message;
  3. request timeout situation;

The later two should not crash the programme like this. Instead, a 500 should (probably) just send a notification and wait a longer amount of time before trying again (or potentially remedy the situation itself). And a request timeout should also wait for a longer time and then attempt it again.

  • 2
    As mentioned in the other review: Timer is usually not the correct class to use for this kind of problem. More isiomatic are constructions with ExecutorService and related – Vogel612 Dec 1 '16 at 13:26

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.