Through the eyes of sonar : Complexity

Complexity

I’m continuing the “Through the eyes of sonar” series ( 1 & 2 ) with various code complexity issues: First a small recap of guidelines/rules and then some practical examples.

KISS Keep it simple & stupid

  1. Easy to read : Simple code doesn’t need additional documentation, or it only needs minimal documentation to be understood
  2. Easy to use : Whoever is using your code will find intuitive to use your objects.
  3. Easy to change : Simplicity means that there is no duplicated logic. A change should match a single place in your code.
  4. Doesn’t use any non-necessary third party tool or technology. Through my experience I have observed how some developers just love to use technologies, tools and frameworks just for the sake of making the project more “cool”. Every time you use them, you are adding extra complexity, and extra complexity means that is more difficult to understand, to maintain and is more prone to failures.
  5. It looks simple. If it doesn’t look simple, it is not simple! You know your code is simple if when you finish you are astonished at how simple the final solution is and you ask yourself how it was possible that it took you so long.
  6. Lean : It only does what is necessary, and nothing else. I think most seasoned developers would agree that trying to anticipate the problems your code will have to face in the future is impossible.
  7. Is straightforward. It doesn’t have unnecessary indirections; the most important operations only require a straight method call.

Some basic rules

Lets see some of the basic rules to write maintainable code :

  1. Keep methods short(one screen limit)
  2. Never ever ever reuse a variable for different purpose
  3. Use self-descriptive variable and method names
  4. Define variables as close as possible to the place of their usage
  5. No magic numbers
  6. Be friend with your language
  7. Don’t fight the convention
  8. Watch out for premature optimization
  9. Always refactor the code after you test it
  10. Don’t get sucked into over-engineering
  11. Learn new things by prototyping

All these rules are explained with concrete examples here

Cyclomatic complexity

Tools like checkstyle will check the cyclomatic complexity of methods against a specified limit.

The complexity is measured by the number of if, while, do, for, ?:, catch, switch, case statements, and operators && and || (plus one) in the body of a constructor, method, static initializer, or instance initializer. It is a measure of the minimum number of possible paths through the source and therefore the number of required tests.

Generally :

  • 1-4 is considered good,
  • 5-7 ok,
  • 8-10 consider re-factoring, and
  • 11+ re-factor now !

How we can reduce it… by implementing simple refactoring. Some of them are built-in in eclipse IDE, sample !

Simplify Boolean Return (Conditional logic can be removed)

Sometime we write unnecessary conditional logic :

public boolean occupiesPosition(int x) {
   if (x >= position && x <= position + data.length())
      return true;
   else
      return false;
}

Can be rewritten :

public boolean occupiesPosition(int x) {
  return (x >= position && x <= position + data.length())
}

Note that some developer still like to introduce a boolean temporary variable to ease the debugging or logging, may be for those developer you can disable UnnecessaryLocalBeforeReturn rule.

Simplify Conditional

Sometime our paranoia of NullPointerException leads us to write unnecessary code. In this example there’s no need to check for null before an instanceof.

public static Object[] getSelectedElements(final ISelection sel) {
   if (sel == null || !(sel instanceof IStructuredSelection))
     return null;
   IStructuredSelection strucSel = (IStructuredSelection) sel;
   ...
}

Extract boolean expression in a method

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  if ( (start <= startTime && end >= startTime) ||
     (start >= startTime && end <= endTime) ||
     (start <= endTime && end >= endTime) ) {
      res.add(timeLine);
  }
}

If you put the logic in a contains method, you end up with 2 methods of lower complexity and easier to read !

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  if ( contains(timeLine)) {
      res.add(timeLine);
  }
}

And with the extracted methods :

private boolean contains(ITimeLine timeLine) {
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  return (start <= startTime && end >= startTime) || (start >= startTime && end <= endTime) || (start <= endTime && end >= endTime) )
}

Replace if else if else by a map or a set

For example sometimes you see a deep if else tree like this

public boolean isManual() {
      if (MANUAL_CLERCK.equals(this))
         return true;
      else if (MANUAL_MEMBER.equals(this))
         return true;
      else if (MANUAL_INSURER.equals(this))
          return true;
      else if (MANUAL_DECL.equals(this))
	     return true;
      return false;
}

Adding a Set may simplify the if-else-if cascade to a simple contains

private static final Set<IndicationOrigin> MANUAL_ORIGINS = new HashSet<IndicationOrigin>();
static {
   MANUAL_ORIGINS.add(MANUAL_CLERCK);
   MANUAL_ORIGINS.add(MANUAL_MEMBER);
   MANUAL_ORIGINS.add(MANUAL_INSURER);
   MANUAL_ORIGINS.add(MANUAL_DECL);
}
public boolean isManual() {
   return MANUAL_ORIGINS.contains(this);
}

Avoid ternary operator

the following code throws a NullPointerException… despite the usage of the ternary operator to check for null value.

DateMidnight startDate = null;
LOGGER.debug(" ==> NPE " + startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null");

In fact the priotiry in operators, will be evaluated like this (added parentheses) :

LOGGER.debug((" ==> NPE " + startDate)!= null ? startDate.toString(Constants.DATE_FORMAT) : "null");

So the boolean expression is always true (concat string) even if the startDate is null.
To fix this you either drop the ternary operator or add the necessary braces to change the evaluation priority

LOGGER.debug(" ==> correct log but full timestamp format " + startDate);
LOGGER.debug(" ==> correct log " + (startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null"));

The code maybe shorter, but you’ve won a bug, so avoid using the ternary operator.

Dodgy – Method uses the same code for two branches

This kind of constructs complexify the code :

if (userLanguage == null || userLanguage.equals("fr")) {
   formattedAddress.append(address.street.trim());
} else {
   formattedAddress.append(address.street.trim());
}

Either you can remove the if else…

formattedAddress.append(address.street.trim());

Either you have a copy/paste error

if (userLanguage == null || userLanguage.equals("fr")) {
     formattedAddress.append(address.street.trim());
 } else {
     formattedAddress.append(address.streetNl.trim());
 }
About these ads
  1. #1 by Rajesh M on November 28, 2012 - 11:56 am

    >>I’m continuing the “Through the eyes of sonar” series ( 1 & 2 )
    Both links point to Exception handling page

    • #2 by mestachs on November 28, 2012 - 3:47 pm

      thanks it’s fixed ;)

  1. Through the eyes of sonar : Complexity | Software Quality - Sonar by SonarSource | Scoop.it
  2. Through the eyes of sonar : Complexity | DevOps in the Enterprise | Scoop.it
  3. Through the eyes of sonar : Naming & Comments. « Don't Make the Same Mistake Twice
  4. Through the eyes of sonar : Collections. | Don't Make the Same Mistake Twice
  5. Did you say Complexity, eh? | 8th color
  6. Through the eyes of sonar: recap. | Don't Make the Same Mistake Twice

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 25 other followers

%d bloggers like this: