9
\$\begingroup\$

Given this kind of code:

public static final int DEFAULT_MASK = Mask.BASE
    | Mask.SECTOR
    | Mask.GROUP
    | Mask.INDUSTRY
    | Mask.NAME
    | Mask.COUNTRY;

Sonar raises a warning:

Boolean expression complexity is 5 (max allowed is 3).

What would be a good way to deal with this?

I suppose the warning might go away if I rewrite like this:

public static final int DEFAULT_MASK;
static {
    int tmp = Mask.BASE;
    tmp |= Mask.SECTOR;
    tmp |= Mask.GROUP;
    tmp |= Mask.INDUSTRY;
    tmp |= Mask.NAME;
    tmp |= Mask.COUNTRY;
    DEFAULT_MASK = tmp;
}

... but this smells (too many mutations, ugly).

UPDATE

In addition to the default mask above, I have a few other masks as well, using different sets of bits, for different purposes. Every bitmask with more than 3 bits would raise a Sonar violation.

\$\endgroup\$
  • 5
    \$\begingroup\$ In a vacuum, I'd say that your original code is better than your workaround. However, to do a proper code review, we need more context to understand the root cause of the code smell. How will this mask be used? \$\endgroup\$ – 200_success Jan 24 '14 at 11:23
  • 5
    \$\begingroup\$ An automated programm will weigh complexity different than a human reader. Here Sonar's mistake is to confuse the bitwise operators on constants (complexity ~ 1) with short-circuiting logical operators on general expressions (complexity ~ n). I did a write-up on such issues over on programmers. \$\endgroup\$ – amon Jan 24 '14 at 11:32
9
\$\begingroup\$

If it’s a complex bitwise operation, like:

result = a & b | c | d & x & y

then it would make perfect sense to raise a violation. On the other hand, this looks like an "innocent" bitmask:

mask = a | b | c | d

An automated tool cannot judge if this is dangerous or not, so it's understandable to raise a flag for this. But in this particular case, it's clearly a false positive.

I put // NOSONAR on the offending line of code to make the warning go away, like this:

public static final int DEFAULT_MASK = Mask.BASE  // NOSONAR
    | Mask.SECTOR
    | Mask.GROUP
    | Mask.INDUSTRY
    | Mask.NAME
    | Mask.COUNTRY;
\$\endgroup\$
4
\$\begingroup\$

The first expression in the OP is better than its alternative; and as amon said in a comment this warning is IMO a mistake by Sonar.

Therefore, I suggest, see Turning Sonar off for certain code on StackOverflow.

\$\endgroup\$
4
\$\begingroup\$

You could do something like the following. It may be better or worse depending on what you want to do:

enum Mask {
    BASE(true),
    SECTOR(true),
    GROUP(true),
    ...
    OTHER(false)
    ;

    private final isDefault;

    private Mask(boolean isDefault) {
        this.isDefault = isDefault;
    }

    public boolean isDefault() { return isDefault; }

    public boolean intValue() { return 1 << ordinal(); }
}

public static final int DEFAULT_MASK = getDefaultMask();

private static int getDefaultMask() {
    int result = 0;
    for (Mask mask : Mask.values()) {
        if (mask.isDefault()) {
            result |= mask.intValue();
        }
    }
    return result;
}
\$\endgroup\$
2
\$\begingroup\$

Without any more information than in your question, this is just a thought. I have no idea what Sonar would say about this approach:

private static final int part1 = Mask.BASE | Mask.SECTOR | Mask.GROUP;
public static final int DEFAULT_MASK = part1 | Mask.INDUSTRY | Mask.NAME | Mask.COUNTRY;

With good naming of the constants, and if the bitmasks are structured in a good way, this approach might actually be preferable.

However, I really don't think this is much of an issue.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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