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;
}
CodePudding user response：
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();
}
}
CodePudding user response：
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;
}
}
CodePudding user response：
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();
}
}
CodePudding user response：
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.