Saturday, January 11, 2014

Confusing Condition Check examples

Our team implemented very impudent Check - ConfusingConditionCheck in release 1.9.0.

In short, This check prevents negation within an "if" expression if "else" is present.
For example, rephrase:
if (x != y) smth1(); else smth2();
as:
if (x == y) smth2(); else smth1();

But we are not that simple !!! in other case Check will not be useful in real live so we have bunch of options...

Options:

multiplyFactorForElseBlock - Allow to ignore "else" block if its length is in
 "multiplyFactorForElseBlocks" time less then "if" block.

ignoreInnerIf - Disable warnings for all "if" that follows the "else". It is useful for
 save similarity with all "if-then-else" statement.

ignoreSequentionalIf - Disable warnings for all sequential "if". warnings for all "if" that follows the "else".

ignoreNullCaseInIf - Disable warnings for "if" if it condition contains "null".

ignoreThrowInElse - Disable warnings for "if" if "else" block contain "throw".


Examples for each option.

our UTs input file is here.

1) Example of Simple IF-ELSE with inversion
that could be swapped in their position and condition could be inverted
String[] result = null;
if (!checkNotExists) {   /// warning is here !!!!
  result = new String[] { updateSql };
} else {
  result = new String[] { updateSql, removeItems(table, column) };
}

another example:
if (!popup.isShowing()) {  /// warning is here !!!!
  show();
} else {
  hide();
}

 another example with complicated condition:
if (!global && part != this) {  // warning is here !!!
  addLocalPart(part);
} else {
  addGlobalInternal(part);
}
code is expected to be, base on De Morgan's laws from Boolean Algebra:
if (global || part == this) { 
  addGlobalInternal(part);
} else {
  addLocalPart(part);
}


2) Example of multiplyFactorForElseBlock=4 .
IF condition is negative but as it big enough(mean that is could be more valuable for developer attention) it should stay in on top so that option skip that case.
  if (!hasWhitespaces(kw)) { //NO warning here   sb.append(kw);
  sb.append('"');
  sb.append('--"');
  sb.append('"');
sb.append('--"'); } else { sb.append(kw); } 
3) example with ignoreNullCaseInIf=true.
Check for NULL is used everywhere in code, and developer by that check highlight that first block is main logic, ELSE block is exceptional processing, so we keep condition without inversion as IF block is more valuable for logic.
if (entity != null) { //NO warning here handler.handleReference(entity); } else { log.error("Some problems with resolving reference"); }

4) Example with ignoreSequentionalIf = true; 
Such pattern is also well spread in java code, unfortunately there it hard to say that after inverting logic IF-ELSE will looks better.
if (!isAllowed(row)) {   //NO warning here
label.setForeground(Color.gray);
} else if (isSelected) {
label.setForeground(Color.white);
} else {
label.setForeground(Color.black);
}
there is even more complicated, so it is user responsibility what conditions should be used and what processing block should located above:
if (obj1 == obj2) {
result = true;
} else if (obj1.getClass() != obj2.getClass()) {   //NO warning here
result = false;
} else {
Class<?> obj1Class = obj1.getClass();
     // a lot of code .......
   }
to see more examples please visit issue at github.

5) Example for ignoreThrowInElse=true;
ELSE block is always treated as processing of non main part of logic, so all unexpected processing and terminations are expected in it more than in IF.
By mean of this option you can skip cases where ESLE is used to process exceptional cases of logic, so main block of logic have to be placed on top.

if (!gzip) {  //NO warning here due to "throw" in ESLE
  stream = new BufferedInputStream(in);
} else {
  log.debug("resource is gzip'd: " + url);
try {
  stream = new BufferedInputStream(new GZIPInputStream(in));
} catch (IOException e) {
  throw new DocumentException(e);
}
}

6) Example of ingnoreInnerIf = true
With that option as "true" this Check will not force you to update following code

if (user != null ) {
   if (obj !=  null ) {
         index++;
   }
} else { 
     index--; 
}

to

if (user == null ) {
    index--;
} else {     
   if (obj !=  null ) {
           index++;
   }
 }

It is some time convenient to keep the same type conditions "!=" close together(close to each other). But it is very debatable what variant is more readable - so it is option :) .

No comments:

Post a Comment