What is better/faster in Java: 2 method calls or 1 object call

Question

I'm afraid this is a terribly stupid question. However, I can't find an answer to it and therefore require some help :)

Let's start with a simplification of my real problem:

Assume I have a couple of boxes each filled with a mix of different gems.

I'm now creating an object gem which has the attribute colour and a method getColour to get the colour of the gem. Further I'm creating an object box which has a list of gems as attribute and a method getGem to get a gem from that list.

What I want to do now is to count all gems in all boxes by colour. Now I could either do something like

int sapphire = 0;
int ruby = 0;
int emerald = 0;

for(each box = i)
    for(each gem = j)
        if(i.getGem(j).getColour().equals(“blue”)) sapphire++;
        else if(i.getGem(j).getColour().equals(“red”)) ruby++;
        else if(i.getGem(j).getColour().equals(“green”)) emerald++;

or I could do

int sapphire = 0;
int ruby = 0;
int emerald = 0;
String colour;

for(each box = i)
    for(each gem = j)
        colour = i.getGem(j).getColour();
        if(colour.equals(“blue”)) sapphire++;
        else if(colour.equals(“red”)) ruby++;
        else if(colour.equals(“green”)) emerald++;

My question is now if both is essentially the same or should one be preferred over the other? I understand that a lot of unnecessary new string objects are produced in the second case, but do I get a speed advantage in return as colour is more “directly” available?

Answer 1

I would dare to make a third improvement:

int sapphire = 0;
int ruby = 0;
int emerald = 0;

for(each box = i) {
    for(each gem = j) {
        String colour = i.getGem(j).getColour();
        if(“blue”.equals(colour)) sapphire++;
        else if(“red”.equals(colour)) ruby++;
        else if(“green”.equals(colour)) emerald++;
    }
}

1. I use a local variable inside the for-loop. Why? Because you probably need it only there.
2. It is generally better to put STATIC_STRING.equals(POSSIBLE_NULL_VALUE).

This has the advantage: easier to read and should have no performance problem. If you have a performance problem, then you should consider looking somewhere else in your code. Related to this: this answer.

Answer 2

conceptually both codes have equal complexity i.e.: O(i*j). But if calling a method and get a returned value are considered to be two processes then the complexity of your first code will be 4*O(i*j).(consider O(i*j) as a function) and of your second code will be O(i*(j+2)). although this complexity difference is not considerable enough but if you are comparing then yes your first code is more complex and not a good programming style.

Answer 3

The cost of your string comparisons is going to wipe out all other considerations in this sort of approach.

You would be better off using something else (for example an enum). That would also expand automatically.

(Although your for each loop isn't proper Java syntax anyway so that's a bit odd).

 enum GemColour {
     blue,
     red,
     green
 }

Then in your count function:

 Map<GemColour, Integer> counts = new EnumMap<GemColour, Integer>(GemColour.class);

 for (Box b: box) {
    for (Gem g: box.getGems() {
       Integer count = counts.get(g.getColour());
       if (count == null) { 
          count=1;
       } else {
          count+=1;
       }

       counts.put(g.getColour(), count);
    }
 }

Now it will automatically extend to any new colors you add without you needing to make any code changes. It will also be much faster as it does a single integer comparison rather than a string comparison and uses that to put the correct value into the correct place in the map (which behind the scenes is just an array).

To get the counts just do, for example:

counts.get(GemColour.blue);

As has been pointed out in the comments the java Stream API would allow you to do all of this in one line:

boxes.stream().map(Box::getGems).flatMap(Collection::stream).collect(groupingBy‌​‌​(Gem::getColour, counting()))

It's less easy to understand what it is doing that way though.