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.
