Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following code and am wondering if this can be further optimized using boolean algebra:

if( ( subType = entry.getSubType() ) ==null
    || 
    !(subType.equals("typeA") || subType.equals("typeB"))
    ||
    ( version>=5 &&  (subType.equals("typeA") || subType.equals("typeB")) )
    ||
    (       version>=4 &&  subType.equals("typeA") )
    ) 
arrayData.add(entry);
share|improve this question

2 Answers 2

I don't really do Java. But this looks like infrequently executed code that you should be optimizing for readability vs. trying to minimize the number of comparisons.

Imagine being in the position of someone maintaining the code after a directive like "we've decided not to support versions prior to 4." Is it completely clear which part they can delete?

So how do you convey the intention of this code (essentially, a version-sensitive decision on whether or not to add an entry to arrayData based on a type?) You want to distinguish between versions prior to 4, version 4, and version 5 and above. Imagine breaking it down so each branch is clearly separated:

boolean shouldAdd = false;

WhateverType subType = entry.getSubType();
boolean isTypeA = (subType == null) ? false : subType.equals("typeA");
boolean isTypeB = (subType == null) ? false : subType.equals("typeB");

if (!isTypeA && !isTypeB)
    shouldAdd = true;

if (version >= 4) { 
    if (isTypeA)
        shouldAdd = true;
}

if (version >= 5) {
    if (isTypeB)
        shouldAdd = true;
}

if (shouldAdd)
    arrayData.add(entry);

This approach tips toward being verbose just to float the idea of leaning away from thinking you have to collapse everything into a single if statement. You can have complex per-version logic that sets a variable, and that variable can trigger the ultimate decision. (In this case your per-version logic is pretty simple, so it's overkill.)

If you cache the result of the type check in a boolean, it will be cheap and avoids potential mistyping of the string constant. (And if you're referring to these frequently, you should probably create a global constant... it's easy to miss and get "tpyeA" and have no compiler checking to catch the mistake!)

share|improve this answer

@HostileFork is completely right. From a code review perspective, your code should be optimized for readability.

It could be a little more DRY.

WhateverType subType = entry.getSubType();
boolean isTypeA = (subType == null) ? false : subType.equals("typeA");
boolean isTypeB = (subType == null) ? false : subType.equals("typeB");

//What should be added ? 
if ( ( !isTypeA && !isTypeB    )  // Anything other than A and B should be 
   ||( version >= 4 && isTypeA )  // Type A for version 4 and over should be
   ||( version >= 5 && isTypeB )) // Type B for version 5 and over should be
  arrayData.add(entry);

Note that this code is 9 lines as well, but readable.

share|improve this answer
1  
+1 for saying I'm right. :-) I wanted to introduce a variable just to show that the per-version logic could be more complex and not force one into a single if statement, but I've expanded and made that more explicit... –  HostileFork Oct 24 '13 at 16:13
2  
"typeA".equals(subType) will allow you to skip the null checks, assuming subType is a string and not a different class with a special equals –  Izkata Oct 24 '13 at 19:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.