Does this construction make sense?

Question

Faced a project with this code:

   public class IndexUpdater implements Runnable {
        @Override
        public void run() {
            final AtomicInteger count = new AtomicInteger(0);
            FindIterable<Document> iterable = mongoService.getDocuments(entryMeta, null, "guid");
                iterable.forEach(new Block<Document>() {
                    @Override
                    public void apply(final Document document) {
                        count.incrementAndGet();
                        // A lot of code....
                        if (count.get() / 100 * 100 == count.get()) {
                            LOG.info(String.format("Processing: %s", count.get()));
                        }
                    }
                });
        }
   }

Here I am interested in three lines of code:

if (count.get() / 100 * 100 == count.get()) {
    LOG.info(String.format("Processing: %s", count.get()));
}

Does this condition make sense considering multithreading and the type of the AtomicInteger variable? Or is this a pointless check? Interestingly, IntellijIdea does not emphasize this construct as meaningless.

Answer 1

I wouldn't call this code "meaningless", but rather wrong (or, it has probably-unintended semantics).

If this were being invoked in a multithreaded way, you wouldn't always get the same value for count.get() on the three invocations in the method (there are four if you include count.incrementAndGet()).

The consequences of this don't look catastrophic in this case - you'd perhaps miss a few logging statements, and you might see some unexpected messages like Processing 101, and then wonder why the number isn't a multiple of 100. But perhaps if you employed the same construct elsewhere, there would be more significant implications.

Put the result of count.incrementAndGet() into a variable (*), so you can use that afterwards.

But then, it would be easier to use count.get() % 100 == 0 as well:

int value = count.incrementAndGet();
// A lot of code....
if (value % 100 == 0) {
  LOG.info(String.format("Processing: %s", value));
}

which is both correct (or, it is probably what is intended) and easier to read.

---

(*) Depending on what you actually want to show with this logging message, you might want to put the count.incrementAndGet() after "A lot of code".