Cyclomatic Complexity – Managing Non-Reusable Functions

code-qualitycomplexitycyclomatic-complexityquality-attributes

I am frequently writing parser and mappers of some sort. This means that the switch statement gets used a lot and often many if statements for checking for valid input.

This of course creates a lot of cyclomatic complexity. I usually split out value parsers. So I have some data format and I have to convert/map the data into a more generic or internal format, often also converting units. So I create functions or methods to convert those.

I care about readability, and the best measurement that people come up with seems to be cyclomatic complexity.

Now I think it might be a great measure for code that you read line by line, but I struggle a bit when it's just clear mappings. An example is the conversion of cardinal direction (so WSW for example) to degrees (247.5°). The function contains a straight forward switch statement, yet it's cyclomatic complexity is really high. That is despite never having anyone that didn't instantly know what this function does, how it works, etc. So I'd argue it's actually one of the most readable functions in the code, yet linters complain about it and similar functions.

I dug a bit more into this and also looked at what others did. Here I noticed what I consider a problem. Measuring cyclomatic complexity on a per function basis leads people to what I consider "cheating". Functions are split into many functions that only work when used with another function.

So developers who want to increase their scores with linters "refactor" the code by essentially breaking up the code in the middle and calling both functions. Each of these functions then have half the cyclomatic complexity, but they must both be called one after another. So it doesn't actually make it easier for anyone to read the function and the split out (private) functions also can't be reused because they are interdependent with the calling function and the second function. This results in a spread of the complexity into three functions, lowering the reported complexity for them, yet not making it any simpler to read.

Sometimes those approaches are well hidden, by interesting sounding function names and by not just having two functions called in a row, but by having multiple functions, things being pretty out of order and all that happens is code with high complexity and a spaghetti code like structure being considered okay by linters, yet being incredibly hard to read by other developers.

My question is whether those things do actually have a benefit that I am missing and if not, whether there is a way to prevent that from happening at linting time.

Would it make more sense to measure it over whole classes, modules, the whole program?

This of course isn't limited to functions, but also methods or classes that only exist to be used together with another class, and require the developer to have a good understanding of the other class to be used at all.

EDIT: What I mean with cheating is not breaking something into individual functions that make sense. Let me give you an example.

func cardinal16ToDegree(dir string) (float64, error) {
    switch strings.ToLower(dir) {
    case "n":
        return 0, nil
    case "nne":
        return 22.5, nil
    case "ne":
        return 45, nil
    case "ene":
        return 67.5, nil
    case "e":
        return 90, nil
    case "ese":
        return 112.5, nil
    case "se":
        return 135, nil
    case "sse":
        return 157.5, nil
    case "s":
        return 180, nil
    case "ssw":
        return 202.5, nil
    case "sw":
        return 225, nil
    case "wsw":
        return 247.5, nil
    case "w":
        return 270, nil
    case "wnw":
        return 292.5, nil
    case "nw":
        return 315, nil
    case "nnw":
        return 337.5, nil
    }

    return 0, errors.New("could not parse cardinal direction")
}

is not more complex than:

func Cardinal16ToDegree(dir string) (float64, error) {
    switch dir {
    case "n":
        return 0, nil
    case "nne":
        return 22.5, nil
    case "ne":
        return 45, nil
    case "ene":
        return 67.5, nil
    case "e":
        return 90, nil
    case "ese":
        return 112.5, nil
    default:
        return cardinalHelper(dir)
    }
    return 0, errors.New("unreachable")
}

func cardinalHelper(dir string) (float64, error) {
  switch dir {
    case "se":
        return 135, nil
    case "sse":
        return 157.5, nil
    case "s":
        return 180, nil
    case "ssw":
        return 202.5, nil
    case "sw":
        return 225, nil
    case "wsw":
        return 247.5, nil
    case "w":
        return 270, nil
    case "wnw":
        return 292.5, nil
    case "nw":
        return 315, nil
    case "nnw":
        return 337.5, nil
  }
  return 0, errors.New("could not parse cardinal direction")
}

This is an extreme case. When I say cheating I mean lowering the score, by taking something out of a function that still always has to be seen in a unit with the other function, as the logic is split, but not encapsulated. So when you want to reason about it there is no way to either only consider one function by seeing the other as a "black box" or by considering the other independent.

This is often also "reached" by creating complex data structures, like a giant state object that gets passed around between functions and classes, and is not following a clear schema. This of course then happens in a language where this is easy to remove an add arbitrary keys and values from non-flat dictionaries. These means that you "hide" spaghetti code, by using an dictionary as a global object or what would be the scope of one big function. Again, here I don't talk about a well-defined object, but merely a way to lower the cyclomatic complexity. That's why I call it cheating.

An example of what I mean by that is this form:

function foo(a, b, c) {
  // many lines of code
  x = ...
  y = ...
  z = ...
  // more code
  result = _foo(a, b, c, x, y, z)
  return result
}

function _foo(a, b, c, x, y, z) {
  // many lines of code that require
  // knowledge of the implementation of
  // foo and that are not a logical
  // separate step, but an extension
  // of foo
  return some_result
}

Again the cyclomatic complexity is considered lower for each of these functions.

Best Answer

Yes, this “cheating” is real, and yes, cyclomatic complexity is not an ideal measure of subjective code simplicity. It is still a very useful code quality indicator.

When we extract a function, we are introducing an abstraction. Any abstraction has a cognitive overhead, it is not “for free”. The art in this refactoring is then choosing abstractions carefully so that understanding the abstraction is simpler than understanding the extracted code. This is likely (but not necessarily) the case when the new abstraction can be used to simplify multiple pieces of code, but even a single-use abstraction can be useful when it is sufficiently powerful.

Arbitrarily breaking a function up does not have any value. That doesn't create any abstractions and just obfuscates the actual control flow. What was now a lot of complexity in one place is now a lot of complexity all over the place: the code has become less cohesive.

Instead of a simple function-based cyclomatic complexity, it would make more sense to calculate the cyclomatic complexity of the part of the code that is unit-tested and understood together. In this sense, it has limited value to introduce private helper functions that offer no abstractions and do not have a well defined interface. Eliminating repetitions would be the only benefit here. In contrast, introducing modules, classes, or functions that can be tested, understood, and documented separately do more likely represent useful abstractions. Including them in the cyclomatic complexity calculation of your code is not useful.

I don't know any tool that does this kind of calculation. As a proxy, I use the testing effort with the goal of high branch coverage for the system under test. When the code is decoupled by the use of good abstractions, anything of interest can be tested fairly directly without much effort. If there are parts of the code that refuse to be covered easily, this might indicate these parts are a candidate for abstraction. This tends to be the case with nested control flow constructs, and also with multiple unrelated sequential conditionals or loops.

Finally, it's worth pointing out that subjective complexity is more important than the numbers produced by a tool. Automatic quality checks are likely to miss many parts that make the code difficult to understand for humans, such as issues with the design or naming, but also overuse of reflection and metaprogramming. And some kinds of code are easier for humans to understand then for the tools. The switch/case construct is the prime example of this. For a human, 20 cases look maybe twice as complicated as 5 cases, as long as the cases are simple and don't interact with each other. Tabular formats are super easy for us to understand! But a linter sees 20 branches, which is way over any acceptable cyclomatic complexity.

For that reason, any linter worth its salt can be selectively turned off for a small code region. Linter warnings are just a suggestion to investigate. If your investigation concludes that the warning was a false positive and that the code is in fact easy to understand, then please don't refactor that function.

Related Topic