Welcome to Software Development on Codidact!
Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.
Comments on Why static code analyzers such as SonarQube indicate a high code complexity for switch statements?
Parent
Why static code analyzers such as SonarQube indicate a high code complexity for switch statements?
During a presentation of a pipeline configuration, a colleague showed a SonarQube integration and one of its reports. A warning was caused by overrunning the max value for the code complexity threshold in a function containing a fairly large switch statement.
Why are switch statements considered too complex and what to do about them?
The problematic code was similar to the one below, but containing about double of the case statements:
enum FooStasus
{
ToBeStarted = 1,
Starting = 2,
OnGoing = 3,
OnHold = 4,
Stopped = 5
}
private static double GetWeight(FooStasus status)
{
switch (status)
{
case FooStasus.ToBeStarted:
case FooStasus.Starting: return 1.0;
case FooStasus.OnGoing: return 2.0;
case FooStasus.OnHold: return 0.2;
case FooStasus.Stopped: return 0;
default: return 0.5;
}
}
Post
I would skip the theoretical part of actually computing the cyclomatic complexity of a switch statement and mention that it can see as a bunch of if statements. Since each if adds to the complexity, the higher the number of cases, the higher the complexity.
While a simple switch such as the one in the question is simple to read and maintain, if it grows way larger, it becomes easier to introduce bugs, even if C# prevents the flow control from passing from one case to another if the case has at least one instruction (i.e. each nonempty case must use break or return).
One way to mitigate this relies on the actual code complexity. If this is a simple mapping, a simple Dictionary can be used. Example:
private static Dictionary<FooStasus, double> _statusMap = new()
{
{ FooStasus.ToBeStarted, 1.0 },
{ FooStasus.Starting, 1.0 },
{ FooStasus.OnGoing, 2.0 },
{ FooStasus.OnHold, 0.1 },
{ FooStasus.Stopped, 0 },
};
private static double GetWeightSimple(FooStasus status) =>
_statusMap.GetValueOrDefault(status, 0.5);
If the logic is more complex, the dictionary can hold Actions or Funcs that might rely on the case value to perform some logic. If they are more than in the following example, it would be wise to define them as separate functions:
private static Dictionary<FooStasus, Func<FooStasus, double>> GetWeightActMap = new()
{
{ FooStasus.ToBeStarted, _ => { Console.WriteLine("To be started"); return 1.0;} },
{ FooStasus.Starting, status => { Console.WriteLine($"Status is {status}"); return 1.0; } },
{ FooStasus.OnGoing, _ => 2.0 },
{ FooStasus.OnHold, _ => 0.1 },
{ FooStasus.Stopped, _ => default },
};
private static double GetWeightAct(FooStasus status) =>
GetWeightActMap.GetValueOrDefault(status)?.Invoke(status) ?? default;
However, as suggested by elgonzo this should be avoided for anything that is a little more complex than my trivial example and use (local) functions instead, since they provide meaning to each case.
Also, whenever the mapping list is fairly long, a table might be more suitable for storage.
1 comment thread