Reduce Cognitive Complexity

Question

I'm having big trouble with reducing cognitive complexity in given piece of code. Could you please give some tips on how to fix this issue? I could reduce it from 24 to 16 using switch, but it is still 16 and I have no options left

 protected override bool Compare(object valueToValidate, object valueToCompare)
    {
        if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
        {
            return true;
        }

        switch (Type.GetTypeCode(valueToCompare.GetType()))
        {
            case TypeCode.DateTime:
                if (DateTime.TryParse(valueToValidate.ToString(), out var valueToValidateDt)
                    && DateTime.TryParse(valueToCompare.ToString(), out var valueToCompareDt))
                {
                    return valueToValidateDt >= valueToCompareDt;
                }

                break;
            case TypeCode.Double:
                if (double.TryParse(valueToValidate.ToString(), out var valueToValidateDouble)
                    && double.TryParse(valueToCompare.ToString(), out var valueToCompareDouble))
                {
                    return valueToValidateDouble >= valueToCompareDouble;
                }

                break;
            case TypeCode.Decimal:
                if (decimal.TryParse(valueToValidate.ToString(), out var valueToValidateDecimal)
                    && decimal.TryParse(valueToCompare.ToString(), out var valueToCompareDecimal))
                {
                    return valueToValidateDecimal >= valueToCompareDecimal;
                }

                break;
            case TypeCode.Int32:
                if (int.TryParse(valueToValidate.ToString(), out var valueToValidateInt32)
                    && int.TryParse(valueToCompare.ToString(), out var valueToCompareInt32))
                {
                    return valueToValidateInt32 >= valueToCompareInt32;
                }

                break;
            case TypeCode.Int64:
                if (long.TryParse(valueToValidate.ToString(), out var valueToValidateInt64)
                    && long.TryParse(valueToCompare.ToString(), out var valueToCompareInt64))
                {
                    return valueToValidateInt64 >= valueToCompareInt64;
                }

                break;

            default: throw new NotImplementedException();
        }

        return false;
    }

@Peter-ReinstateMonica It would need a lot more context before it is. Besides, they don't want a review. They have a specific problem (cognitive complexity too high), can this be specifically answered on SO? Wouldn't surprise me if it was a duplicate. — Mast, Jan 18 '23 at 07:35
@Mast Well, the question as such (generally about complexity, and specifically) is in the top percentile of interesting questions as far as I'm concerned, and certainly can be answered. — Peter - Reinstate Monica, Jan 18 '23 at 07:49
Do you perhaps mean [**Kolmogorov** complexity](https://en.wikipedia.org/wiki/Kolmogorov_complexity)? — Karl Knechtel, Jan 18 '23 at 08:01
Interesting that you find the code complex -- its redundancy is what bothers me more. The redundancy makes it hard for me to read and verify (because it's likely cut+pasted I must fear that, say, the code for the Double case accidentally tries to parse an int, etc.). Making sure it isn't is tedious and tiresome. By contrast, the logical structure is fairly clear. (The sanity check on top is weird though: Why would `valueToValidate` be larger-or-equal (which is what the function checks) if *either one* of the values is a blank? — Peter - Reinstate Monica, Jan 18 '23 at 08:12

SomeBody · Accepted Answer · 2023-01-18T07:22:08.970

You could try to move the code inside the case blocks into their own methods:

private bool CompareDateTime(string value1, string value2)
    {
        if (DateTime.TryParse(value1, out var valueToValidateDt)
                    && DateTime.TryParse(value2, out var valueToCompareDt))
                {
                    return valueToValidateDt >= valueToCompareDt;
                }
        return false;
    }

Your Comapre would simplify to this:

protected override bool Compare(object valueToValidate, object valueToCompare)
    {
        if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
        {
            return true;
        }

        switch (Type.GetTypeCode(valueToCompare.GetType()))
        {
            case TypeCode.DateTime:
                return CompareDateTime(valueToValidate.ToString(), valueToCompare.ToString());
            case TypeCode.Double:
                 return CompareDouble(valueToValidate.ToString(), valueToCompare.ToString());
            case TypeCode.Decimal:
                 return CompareDecimal(valueToValidate.ToString(), valueToCompare.ToString());
            case TypeCode.Int32:
                 return CompareInt32(valueToValidate.ToString(), valueToCompare.ToString());
            case TypeCode.Int64:
                 return CompareInt64(valueToValidate.ToString(), valueToCompare.ToString());

            default: throw new NotImplementedException();
        }
    }

Because all your methods have the same signature, you could also use a Dictionary<TypeCode,Func<string,string,bool>> instead of a switch block:

protected override bool Compare(object valueToValidate, object valueToCompare)
    {
        if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
        {
            return true;
        }
    var methods = new Dictionary<TypeCode,Func<string,string,bool>>
    {
       { TypeCode.DateTime, CompareDateTime },
       { TypeCode.Double, CompareDouble },
       { TypeCode.Decimal, CompareDecimal },
       { TypeCode.Int32, CompareInt32 },
       { TypeCode.Int64, CompareInt64 }
    };
    if(methods.TryGetValue(Type.GetTypeCode(valueToCompare.GetType()), out var method))
    {
       return method.Invoke(valueToValidate.ToString(), valueToCompare.ToString());
    }
    else
    {
       throw new NotImplementedException();
    }
}

What bothered me about the original code was less the complexity (in fact, the code is rather straightforward) than the repetitiveness. The dictionary is one way to address that. But I wonder whether you can build a generic function because the types must be comparable and parsable. — Peter - Reinstate Monica, Jan 18 '23 at 08:00

score 2 · Answer 2 · answered Jan 18 '23 at 09:03

You may be able to entirely eliminate the switch and the associated redundancy if you can pass concrete types to a generic function like the following. The class contains a perfunctory test in Main.

In general, switches over types should raise suspicions because they try to manually re-invent polymorphism, either compile-time (as here) or runtime polymorphism. It's often better to use the language mechanisms for that.

public class VariableCompare
{
    public static bool Compare<N>(N valueToValidate, N valueToCompare) where N: IComparable<N>, IParsable<N>
    {
        N lhs, rhs;

        if (    N.TryParse( valueToValidate.ToString(), 
                            null, 
                            out lhs )
             && N.TryParse( valueToCompare.ToString(), 
                            null,                            
                            out rhs) )
        {
            return lhs.CompareTo(rhs) >= 0;
        }
        return false;
    }

    static int Main()
    {
        int i1 = 1, i2 = 2;
        Console.WriteLine(Compare(i1, i2));

        float f1 = 2, f2 = 1;
        Console.WriteLine(Compare(f1, f2));

        DateTime d1 = DateTime.Now;
        DateTime d2 = DateTime.Today;
        Console.WriteLine(Compare(d1, d2));

        return 0;
    }
}

tymtam · Answer 3 · 2023-01-18T09:59:36.573

I think for this method formatting and short names can make a world of difference. Here are two versions that I think are very easy to read and to maintain:

Version 1

bool Compare(object valueToValidate, object valueToCompare)
{
    if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
    {
        return true;
    }

    var a = valueToValidate.ToString();
    var b = valueToCompare.ToString(); 

    return Type.GetTypeCode(valueToCompare.GetType()) switch
    {
        TypeCode.DateTime =>      
                   DateTime.TryParse(a, out var x) 
                && DateTime.TryParse(b, out var y)
                && x >= y,
        TypeCode.Double =>
                   double.TryParse(a, out var x)
                && double.TryParse(b, out var y)
                && x >= y,
        TypeCode.Decimal =>  
                   decimal.TryParse(a, out var x)
                && decimal.TryParse(b, out var y)
                && x >= y,
        TypeCode.Int32 => 
                   int.TryParse(a, out var x)
                && int.TryParse(b, out var y)
                && x >= y,           
        TypeCode.Int64 =>
                   long.TryParse(a, out var x)
                && long.TryParse(b, out var y)
                && x >= y,
        
        _ =>  throw new NotImplementedException();
    };
}

Version 2

bool Compare(object valueToValidate, object valueToCompare)
{
    if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
    {
        return true;
    }

    var a = valueToValidate.ToString();
    var b = valueToCompare.ToString(); 

    switch (Type.GetTypeCode(valueToCompare.GetType()))
    {
        case TypeCode.DateTime:
        {
            return DateTime.TryParse(a, out var x) 
                && DateTime.TryParse(b, out var y)
                && x >= y;
        }
        case TypeCode.Double:
        {
            return double.TryParse(a, out var x)
                && double.TryParse(b, out var y)
                && x >= y;
        }
        case TypeCode.Decimal:
        {
            return decimal.TryParse(a, out var x)
                && decimal.TryParse(b, out var y)
                && x >= y;
        }
        case TypeCode.Int32:
        {
            return int.TryParse(a, out var x)
                && int.TryParse(b, out var y)
                && x >= y;           
        }
        case TypeCode.Int64:
        {
            return long.TryParse(a, out var x)
                && long.TryParse(b, out var y)
                && x >= y;
        }
        default: throw new NotImplementedException();
    }
}

tymtam · Answer 4 · 2023-01-18T10:27:10.130

Could you use IComparable?

For example:

bool Compare(object valueToValidate, object valueToCompare)
{
    if (RaUtils.IsBlankValue(valueToValidate) || RaUtils.IsBlankValue(valueToCompare))
    {
        return true;
    }

    if( valueToValidate is IComparable x && valueToCompare is IComparable y)
        return x.CompareTo(y) > 0;

    throw new NotImplementedException();
}

If you want/need keep the switch checking the types can be made simpler (which also removes the ToString/Parse):

bool Compare(object valueToValidate, object valueToCompare)
{
    ...
    return (valueToValidate, valueToCompare) switch
    {
        (DateTime a, DateTime b) => a >= b,      
        (double a, double b) => a >= b,             
        //...
        
        _ =>  throw new NotImplementedException()
    };
}

If you really really really have to round trip of ToString->Parse then in .NET7 you could use the IParseable...er...I mean IParsable interface, part of the Generic Math feature. Could something like this work?

using System.Numerics;

bool CustomCompare<T>(object a, object b) where T: IComparable<T>, IParsable<T>
{
    return T.TryParse(a.ToString(),System.Globalization.CultureInfo.CurrentCulture, out var x)
        && T.TryParse(b.ToString(), System.Globalization.CultureInfo.CurrentCulture, out var y)
        && x.CompareTo(y) > 0;     
}

This could then be used in you switch.

Reduce Cognitive Complexity

4 Answers4

Version 1

Version 2