CODEWITHSUNDEEP
javastatic-analysis
igr
igr

Reputation: 10604

Cyclomatic Complexity, joining conditions and readability

Consider the following method (in Java - and please just ignore the content):

public boolean equals(Object object) {
    if (this == object) {
        return true;
    }
    if (object == null) {
        return false;
    }
    if (getClass() != object.getClass()) {
        return false;
    }
    if (hashCode() != object.hashCode()) {
        return false;
    }
    return true;
}

I have some plugin that calculates: eV(g)=5 and V(g)=5 - that is, it calculates Essential and common CC.

Now, we can write the above method as:

public boolean equals2(Object object) {
    if (this == object) {
        return true;
    }
    if (object == null || getClass() != object.getClass()) {
        return false;
    }
    return hashCode() == object.hashCode();
}

and this plugin calculates eV(g)=3 and V(g)=3.

But how I do understand CC, the values should be the same! CC is not about counting the lines of code, but the independent paths. Therefore, joining two if in one line does not really reduces CC. In fact, it only can make things less readable.

Am I right?

EDIT

Forgot to share this small convenient table for calculating CC quickly: Start with a initial (default) value of one (1). Add one (1) for each occurrence of each of the following:

EDIT 2

I proved that my plugin is not working well, since when I inline everything in one line:

public boolean equals(Object object) {
    return this == object || object != null && getClass() == object.getClass() && hashCode() == object.hashCode();
}

it returns CC == 1, which is clearly wrong. Anyway, the question remains: is CC reduced

[A] 5 -> 4, or

[B] 4 -> 3

?

Upvotes: 2

Views: 614

Answers (4)

lp_
lp_

Reputation: 1178

Long story short...

Your approach is a good approach to calculate CC, you just need to decide what you really want to do with it, and modify accordingly, if you need so.

For your second example, both CC=3 and CC=5 seem to be good.

The long story...

There are many different ways to calculate CC. You need to decide what is your purpose, and you need to know what are the limitations of your analysis.

The original definition from McCabe is actually the cyclomatic complexity (from graph theory) of the control flow graph. To calculate that one, you need to have a control flow graph, which might require a more precise analysis than your current one.

Static analyzers want to calculate metrics fast, so they do not analyze the control flow, but they calculate a complexity metric that is, say, close to it. As a result, there are several approaches...

For example, you can read a discussion about the CC metric of SonarQube here or another example how SourceMeter calculates McCC here.

What is common, that these tools count conditional statements, just like you do. But, these metrics wont be always equal with the number of independent execution paths... at least, they give a good estimation.

Two different ways to calculate CC (McCabe and Myers' extension):

V_l(g) = number of decision nodes + 1
V_2(g) = number of simple_predicates in decision nodes + 1

If your goal is to estimate the number of test cases, V2 is the one for you. But, if you want to have a measure for code comprehension (e.g. you want to identify methods that are hard to maintain and should be simplified in the code), V1 is easier to calculate and enough for you.

In addition, static analyzers measure a number of additional complexity metrics too (e.g. Nesting Level).

Upvotes: 3

meriton
meriton

Reputation: 70564

As far as style is concerned, I consider the following the most readable:

public boolean equals(Object object) {
    return this == object || (object != null && eq(this, object));
};

private static boolean eq(Object x, Object y) {
    return x.getClass() == y.getClass()
        && x.hashCode() == y.hashCode(); // safe because we have perfect hashing
}

In practice, it may not be right to exclude subclasses from being equal, and generally one can not assume that equal hash codes imply equal objects ... therefore, I'd rather write something like:

public boolean equals(Object object) {
    return this == object || (object instanceof MyType && eq(this, (MyType) object));
}

public static boolean eq(MyType x, MyType y) {
    return x.id.equals(y.id);
}

This is shorter, clearer in intent, just as extensible and efficient as your code, and has a lower cyclomatic complexity (logical operators are not commonly considered branches for counting cyclomatic complexity).

Upvotes: 0

Paul Hicks
Paul Hicks

Reputation: 13999

Converting this

if (hashCode() != object.hashCode()) {
    return false;
}
return true;

to this

return hashCode() == object.hashCode();

obviously reduces CC by one, even by your quick table. There is only one path through the second version.

For the other case, while we can't know exactly how your plugin calculates those figures, it is reasonable to guess that it is treating if (object == null || getClass() != object.getClass()) as "if a non-null object's class matches then ...", which is a single check and thus adds just one to CC. I would consider that a reasonable shortcut since null checks can be rolled up into "real" checks very easily, even within the human brain.

My opinion is that the main aim of a CC-calculating IDE plugin should be to encourage you to make your code more maintainable by others. While there is a bug in the plugin (that inlined single-line conditional is not particularly maintainable), the general idea of rewarding a developer by giving them a better score for more readable code is laudable, even if it is slightly incorrect.

As to your final question: CC is 5 if you strictly consider logical paths; 4 if you consider cases you should consider writing unit tests for; and 3 if you consider how easy it is for someone else to quickly read and understand your code.

Upvotes: 2

Mr_Thorynque
Mr_Thorynque

Reputation: 2002

In the second method return hashCode() == object.hashCode(); costs 0 so you win 1. It's considered as calculation and not logical branch. But for the first method I don't know why it's cost 5, I calculate 4.

Upvotes: 1

Related Questions