5

I have following kind of scenario: It is simple form of code that I am showing here to clarify my concern:

public void callMe()
{
 AtomicInteger howManyOdds = new AtomicInteger(0);
 AtomicInteger howManyEvens = new AtomicInteger(0);
 loopThrough(100,howManyOdds,howManyEvens);
 System.out.println(howManyOdds.get()+" "+howManyEvens.get());
}
private void loopThrough(int counter,AtomicInteger howManyOdds,AtomicInteger howManyEvens)
{
 for(int i = 1 ; i <= counter ;i++)
 {
  if(i%2 == 0)
  howManyEvens.getAndAdd(1);
  else
  howManyOdds.getAndAdd(1);
 }
}

I know that it can be done by int[] but it looks kind of odd. Is AtomicInteger a good substitute for Mutable Integer in such kind of cases? If No then WHY?

Mac
  • 1,711
  • 3
  • 12
  • 26
  • 2
    AtomicInteger is built for concurrency. For this use case taking two integers and incrementing them should be more than enough. – Amit Apr 21 '14 at 16:34
  • @Amit So according to you AtomicInteger should not be used in those cases where Concurrency is not involved. My question is Why???? – Mac Apr 21 '14 at 16:37
  • 1
    @Mac It is a bit hacky and not necessarily clear to the reader, so I don't see any problems as long as it is contained in your code (i.e. private and part of your implementation). I would not expose it in a public API though and would use a custom object as proposed by dasblinkenlight instead. – assylias Apr 21 '14 at 16:42
  • @assylias: Thanks for your input. Ok I have set the accessibility of method as private. Now how much is it going to affect the performance of Code or program? – Mac Apr 21 '14 at 16:47
  • 1
    @Mac the performance impact is due to the use of volatile in AtomicInteger but will be minimal (a volatile read is typically equivalent to a non-volatile read - volatile writes can be several orders of magnitude slower than non volatile writes but we are talking about nanoseconds). If you really want to use a mutable structure an `int[2]` would perform better. – assylias Apr 21 '14 at 16:47
  • @assylias So it means that just for the readability purpose and for not alarming the readers for the concurrency smell in code `AtomicXXX` should be avoided in such cases.? – Mac Apr 21 '14 at 17:00
  • 1
    @Mac I've done it in some occasions where it was simpler than anything else and added a comment - in your case because you have two related numbers, using a class would probably improve readability. In the end you need to ask yourself who will read the code in the future and will it be clear or not. It is highly subjective. – assylias Apr 21 '14 at 17:03
  • Thanks @assylias I am more clear now about when to choose such kind of approach. – Mac Apr 21 '14 at 17:08

1 Answers1

8

I do not think this is a good idea: using AtomicInteger in contexts that are not inherently concurrent is misleading to the reader.

Using an array is not a good idea, either, even though technically it works well. The problem is that the resultant code is not too descriptive, in the sense that the "mapping" of indexes to their meanings (i.e. 0 -> odd, 1 -> even) is not visible from the API itself.

You would be better off with a mutable class that holds two properties:

public class OddEven {
    int odd, even;
    public int getOdd() {return odd;}
    public int getEven() {return even;}
    public void incOdd() {odd++;}
    public void incEven() {even++;}
}

This achieves a very good readability, without creating a false impression that something concurrent is going on behind the scene.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Thanks for you input. Yes I would have done in the same way as you have shown here. But , my primary concern is about If it is bad practice to use `AtomicInteger` in non-concurrent cases.. then WHY? I don't see any `synchronization` around any of the methods of `AtomicXXX` methods.. How and Why is it going to affect the performance of code? – Mac Apr 21 '14 at 16:44
  • 2
    @Mac It is not the performance that is of a concern: your code with `AtomicInteger` will be as or nearly as efficient as it gets. The main problem is getting your ideas across to the human readers. It is them why would be scratching their heads and thinking: "why does he wants me to pass `AtomicInteger`? I did not think that his class was concurrent; am I wrong on that?" In general, you want to avoid incidents like that, because they make it hard to maintain your code by others. – Sergey Kalinichenko Apr 21 '14 at 16:47
  • That's nice point that I got to know from you . But if that method is private and is used internally by that class even then you will say that it is the wrong practice ?? – Mac Apr 21 '14 at 17:01
  • 2
    @Mac Yes, even then I think it's a bad practice. At some point you want to move on, and give the code that you wrote to someone else to maintain. You do not want to maintain that code forever, right? Even if you are fine with it, your company may have some doubts (i.e. the notorious "hit by a bus" problem). – Sergey Kalinichenko Apr 21 '14 at 17:06