Cyclomatic Complexity spread over non-reusable functions

Question

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.

Answer 1

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.

Answer 2

Function Reuse

First and formost, functions are a prinicple means of abstraction (SICP) not necessarily a means of reuse. The main problem within software engineering is tackling complexity in the application domain. By limiting function complexity (either cyclomatic, by lines of code (LOC) length or other measures), it provides readers of the code with a better ability to understand the current function. As humans have a limited working memory, reducing complexity aids in maintainability and readability of a program.

By providing a descriptive named procedure or function in place of a series of statements, it gives readers of your code a better means by which they can hook their mental model:

• The data which is being manipulated in the abstracted operation is explicitly declared.
• The group of statements is given a mental abstraction in the form of the function identifier.

From a practical standpoint, function inlining largely addresses any issues with non-reusable functions.

As with all things of course, there is a balance. Unfortunately, a good amount of code out there is skewed towards poor readability via its underuse of abstraction.

Cyclomatic Complexity

Cyclomatic Complexity helps gives a measure of readability in that it shows the the number of control paths one may have to keep in their working memory while reading a function. Although it's not perfect, it is in my opinion one of the better measures out there of code complexity and a good indication that a function probably needs refactored to be easier to trace.

Cyclomatic complexity has known issues with switch cases (see reference on cognitive complexity as an alternative measure). Although the control paths through a switch case may be huge, the condition upon which the paths are taken is easily understood (if the switch case is well written). Switch-cases are heavily used because compilers often optimize them for efficiency in size and time (especially in C/C++). Here are couple of guidelines when utilizing switch cases to improve readability in spite of their high complexity values.

Wrap case bodies

When more than a single line, wrapping case bodies with inline functions (or even just functions) often helps readability and clarity. First and foremost, it forces the programmer to cleanly separate state/variables used in the case statements into function parameters. Secondly, it avoids making the function length incredibly large. Function lengths great than 100 LOC are often a source bugs and difficult to refactor/maintain (Code Complete).

If this is not possible, at least try to provide curly braces around the switch body to delineate and scope the case body.

Do Not Declare Variables Before First Case Statement

This one follows directly from a CERT Secure Coding Standard Rule (CERT DCL41-C). It's pretty self explanatory and intuitive, but in from experience, programmers are prone to doing hideous things in switch cases.

Avoid Weird Control Flow

Things like Duff's Device are detrimental on readability and break a good number of decent coding standards (i.e. CERT, MISRA). This includes using "clever" tricks like statement fall through for case bodies (by omitting break; at the end of a statement). Fall through is trivial to avoid if you utilize functions for case bodies.

Const or Define case values

This is a MISRA hold over to avoid magic numbers, but where it makes sense, avoid throwing a bunch of constant literals in case labels. Wrap them in meaningful #define or const identifiers so that someone looking at the code has context as to what you're switching.

Alternatives

You can sometimes get rid of a switch case entirely, thus avoiding the cyclomatic complexity.

Use a Jump Table if Case Expression Values are Contiguous

Sometimes you can avoid a switch-case all together. If the case expression values are contiguous in nature and the case bodies are distinct enough, you can use a constable array of pointers to functions to call (in C/C++).

In some more dynamic languages with hashes, you may be able to hash the value to a set of functions.

If-Else

Sometimes, switch cases are overused. If you're using a switch case with only a couple (2-3) case values, odds are the switch block can be rewritten as a series of simple if-else blocks with similar performance.

Examples

Bad

int f;
switch(packet[0]) {
  case 0x8A:
    f = packet[7] + packet[2];
    break;
  case 0xBf:
    f = packet[6] + packet[3];
  case 0xAf:
    f = packet[2] + packet[8];
 }

Good

status_t status = STATUS_INVALID;

switch(packet_id)
{
  case PACKET_ID_ACKNOWLEDGEMENT:
  {
    status = packet_handle_acknowledge(packet);
    break;
  }
  case PACKET_ID_TEST_RESPONSE:
  {
    status = packet_handle_test_response(packet);
    break;
  }
  default:
  {
    status = packet_default_handle(packet);
    break;
  }
}

Alternative to Switch-Case Example

In the example below, a table packetHandlers for dispatching packets based of an id is shown. If the PACKET_ID values for this were contiguous in nature and the PacketHandler_t functions were different enough, this may be sufficient. However, a switch would probably be better in code size if there were a significant number of of the same PacketHandler_t function being used for ranges of PACKET_IDs. The Packet_Dispatch function here however does have a lower complexity than a switch case.

#include <stdint.h>
#include <assert.h>

#define PACKET_PAYLOAD_ID_INDEX (0U)
#define PACKET_BYTE_SIZE (8U)

#define PACKET_ID_ACKNOWLEDGEMENT (0U)
#define PACKET_ID_TEST (1U)
#define PACKET_ID_UPPER_RANGE_LIMIT (2U)

#define STATUS_OK (0)
#define STATUS_FAIL (-1)

typedef struct Packet_s
{
   uint8_t payload[PACKET_BYTE_SIZE];
}
Packet_t;

typedef int8_t Status_t;

typedef Status_t (*PacketHandler_t)(const Packet_t *p);

Status_t PacketHandler_Acknowledge(const Packet_t *p)
{
  /* Perform acknowledgement processing */
  return STATUS_OK;
}

Status_t PacketHandler_Default(const Packet_t *p)
{
  /* Perform default packet handling */
  return STATUS_OK;
}

static const PacketHandler_t packetHandlers[] =
{
  [PACKET_ID_ACKNOWLEDGEMENT] = PacketHandler_Acknowledge,
  [PACKET_ID_TEST] = PacketHandler_Default
};

Status_t Packet_Dispatch(const Packet_t *p)
{
  Status_t status = STATUS_FAIL;
  uint8_t packet_id = p->payload[PACKET_PAYLOAD_ID_INDEX];

  if(packet_id < PACKET_ID_UPPER_RANGE_LIMIT)
  {
    PacketHandler_t handler = packetHandlers[packet_id];
    status = handler(p);
  }

  return status;
}

References

• Cognative Complexity - SonarQube's new complexity measure that deals with some shortfalls of cyclomatic complexity.
• CERT Secure Coding Standard - Great coding standard for secure coding practices.
• Code Complete by Steve McConnell - good book with practical advice for software engineering
• MISRA C:2012 Guidelines for the use of the C language in critical systems
• (SICP) - Structure and Interpretation of Computer Programs

Answer 3

I advocate measuring CC across an individual method. I do not believe breaking up large methods into smaller methods is "cheating".

The smaller methods may permit: (1) better naming, (2) proper separation between logical steps, (3) testing, and (4) code reuse. Most importantly, smaller methods are easier to understand and this is exatly what cyclomatic complexity is trying to get at.

The argument that breaking up methods is "cheating" implies that aggregate measures are better. Relying solely on aggregate metrics does not help you identify problems in the code. For example, if I tell you the total run-time of a program is 1 hour do you know which component of the program took 45 minutes?

All this said, there is an argument to make about "scoring" a Class based on its "Average Cyclomatic Complexity per method". We just need to be aware that average cyclomatic complexity is quickly skewed by many tiny getters and setters.