Optimize/Beautify Boolean Statement in Specific Code [closed]

Question

I have the following code and am wondering if this can be further optimized and also beautified (in order to be more readable) :

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);

Answer 1

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

Answer 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!)