Writing efficient Java

[Amandeep Singh]

SnapShot of the code

boolean conditionMBL = getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[0]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[0]));
boolean conditionHBL = getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[1]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[1]));
boolean condtionUOM = getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[3]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[3]));

				if (conditionMBL && conditionHBL && condtionUOM){

or

 if (
				getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[0]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[0]))
				&& getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[1]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[1]))
					&& getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[3]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[3]))
			)    

both are different ways of doing it. and it is guranteed this code will be executed only 1 time.
I am preferring 2nd because it is not using unnecessary boolean's.
whereas in 1st, it looks more clean.

can you let me know the final answer 1 or 2 ?

[Cameron Wallace McKenzie]

Neither one looks particularly handsome.

My preference is always for what is going to be easiest to maintain in the long term. I'd prefer to lose a clock cycle, than lose a day trying to update or troubleshoot some incredibly efficient code. But in this case, option two doesn't look like a huge readability improvement over case 1. I'd stick with the second.

-Cameron McKenzie

[Yaroslav Chinskiy]

I vote for 1.

I think that readability of the code is more important than marginal optimization.
I am not an expert on JVM but most likely compiler or JVM will optimize performance anyway.

If code is hard to read/maintain it will lead to degrading performance in the long run....

[Garrett Rowe]

How about a third choice:

if (isValid(nPosDB, mPosDB, manifestColumns)) {
   //...
}


private boolean isValid(...) {
   return  getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[0]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[0]))  
                 && getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[1]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[1]))  
                 && getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[3]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[3])) 
}
   

[Amandeep Singh]

the third choice is modular. i would prefer third choice if this code is used more than 2 times.

[Cameron Wallace McKenzie]

Cameron Wallace McKenzie wrote:Neither one looks particularly handsome.

-Cameron McKenzie

By the way, I wasn't saying it was bad code. It was more a comment on the visual aesthetics.

-Cameron McKenzie

[Ernest Friedman-Hill]

I would go with something similar to Garrett's suggestion:

if (isValid(nPosDB, mPosDB, manifestColumns, 0) && isValid(nPosDB, mPosDB, manifestColumns, 1) && isValid(nPosDB, mPosDB, manifestColumns, 2) ) {
   //...
}

private boolean isValid(...) {
   return  getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[column]).equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[column]);
}

It's fine to use an extra method like that even in just this one place as long as helps in making the code more readable, testable, accurate, or maintainable. It does all four of those things!

[Amandeep Singh]

Awesome Ernest.

i like your all points. less bugs. have to look at 1place only.

this thing should have come to my mind...

[Duc Vo]

This is another variant from Ernest's solution. It's because the column index doesn't have any meaning outside the array anifestColumns.

if (isValid(nPosDB, mPosDB, manifestColumns[0]) 
      && isValid(nPosDB, mPosDB, manifestColumns[1]) 
      && isValid(nPosDB, mPosDB, manifestColumns[2]) ) {  
    //...  
}  

private boolean isValid(...) {  
    return  getTrimmedUCaseDBColumnValue(nPosDB, manifestColumn).equals(
                   getTrimmedUCaseDBColumnValue(mPosDB, manifestColumn)
               );  
}  

[Amandeep Singh]

making it logical..

int i = 0;
    if (isValid(nPosDB, mPosDB, manifestColumns[i++]) 
      && isValid(nPosDB, mPosDB, manifestColumns[i++]) 
      && isValid(nPosDB, mPosDB, manifestColumns[i++]) ) {  
    //...  
}  

private boolean isValid(...) {  
    return  getTrimmedUCaseDBColumnValue(nPosDB, manifestColumn).equals(
                   getTrimmedUCaseDBColumnValue(mPosDB, manifestColumn)
               );  
}  

[Mike Simmons]

For those who missed it, the original index sequence was 0, 1, 3, not 0, 1, 2. So let's fix that while simplifying further:

 
  private static boolean areColumnsValid(int... indices) {
    for (int i : indices)
      if (!getTrimmedUCaseDBColumnValue(nPosDB, manifestColumns[i])
          .equals(getTrimmedUCaseDBColumnValue(mPosDB, manifestColumns[i])))
        return false;
    return true;
  }

  ...

    if (areColumnsValid(0, 1, 3)) {
      ...
    }

I would also be strongly tempted to replace getTrimmedUCaseDBColumnValue(x, manifestColumns[ i ]) with a more meaningful (and hopefully shorter) name - but I have no idea what the significance of this method is. The name tells me how the value was obtained, more or less, but gives me no idea what it means. And I would be much more interested in the latter.

[Jesper de Jong]

Note that of those two options mentioned in the first post, they do not work exactly the same, and the second one is potentially more efficient!

The && operator in Java is a short-circuit operator. That means that if you write A && B and A is true, then Java already knows that the answer to the expression A && B is true, so it is not going to evaluate B.

In your first example, the JVM is going to evaluate all three expressions first, and after that it combines them in the if-statement. Note that all three expressions are always evaluated, regardless of which ones are true or false.

In your second example, the JVM evaluates the first expression and stops if it is true (it doesn't need to evaluate the second and third expression).

[Joanne Neal]

Jesper Young wrote:The && operator in Java is a short-circuit operator. That means that if you write A && B and A is true, then Java already knows that the answer to the expression A && B is true, so it is not going to evaluate B.

Don't you mean 'if you write A && B and A is false, then Java already knows that the answer to the expression A && B is false' ?

[Ernest Friedman-Hill]

Amandeep Singh wrote:making it logical..

Nope. The reader is now wondering what "i" represents, and what the state of "i" at the end of this expression represents, and why we saved "i", and where we're going to use it again.

Mike Simmons wrote:So let's fix that while simplifying further:

My first thought was that this one is extremely nice and clean; then I realized that it's because you blew off several of the arguments. It's still not bad, but to be fair, with out more refactoring, it'll look like this, which doesn't have the same punch:

    if (areColumnsValid(nPosDB, mPosDB, manifestColumns, 0, 1, 3)) {
      ...
    }

[Mike Simmons]

EFH wrote:My first thought was that this one is extremely nice and clean; then I realized that it's because you blew off several of the arguments. It's still not bad, but to be fair, with out more refactoring, it'll look like this, which doesn't have the same punch:

True - I was figuring I'd probably move those to fields. (If they weren't already.) But it depends what scope they naturally have. Chances are good I'd create a class that matches their lifetime, if there wasn't already a class that did so.