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.

I don't understand what `count.get() / 100 * 100 == count.get()` is supposed to accomplish. Are you just trying to check if `count.get()%100==0`? — khelwood, Jan 22 '20 at 08:52
You won't always get the same value for `count.get()` on the two invocations. It would be easier to use `count.get() % 100 == 0`. — Andy Turner, Jan 22 '20 at 08:54
It doesn't make sense considering the practice of "writing code that does what you need": it rounds down to the nearest 100 and then prints if the rounded number is the original number, so... why not print when `count % 100 == 0`, which is the standard way to test for multiples-of? — Mike 'Pomax' Kamermans, Jan 22 '20 at 08:54
*considering multithreading*: is there multithreading here? Does forEach really execute the Block on multiple threads? — JB Nizet, Jan 22 '20 at 08:56
"Interestingly, IntellijIdea does not emphasize this construct as meaningless" Intellij can't possibly determine how meaningful arbitrary code is. It will only flag "known" patterns as such. — Andy Turner, Jan 22 '20 at 08:59

Andy Turner · Accepted Answer · 2020-01-22T19:13:36.923

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".

There's still a race condition, since the OP would call incrementAndGet(), and then get(). The only call should be incrementAndGet(). — JB Nizet, Jan 22 '20 at 09:22

Does this construction make sense?

1 Answers1