current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite

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.

share|improve this question
5  
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? –  200_success♦ Jan 24 at 11:23
5  
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. –  amon Jan 24 at 11:32
add comment

4 Answers

up vote 7 down vote accepted

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 valid to raise a flag. But in my case, this is 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;
share|improve this answer
add comment
up vote 4 down vote

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.

share|improve this answer
add comment
up vote 4 down vote

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;
}
share|improve this answer
add comment
up vote 2 down vote

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.

share|improve this answer
add comment

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.