I wrote an answer to this question on the Software Engineering SE site and was hoping to get it critiqued. (I'm told that it's acceptable to do so).
Essentially, this code uses reflection to check to ensure that none of the parameters of a method are null
and none of the strings are empty or consist only of whitespace. If one of those conditions is violated, it raises an exception on behalf of the caller.
The weakness of this code is that it assumes that it's never valid for a string to consist of whitespace or be empty and that none of the arguments can be null
.
My concerns:
- Is this overly convoluted/difficult to understand?
- Obviously any time you use reflection that's going to entail a performance hit. For the particular system I'm considering using this in, performance isn't a major concern, but if I end up using it in future projects, how much of a performance hit can I expect?
- Overall, is it actually worth using? Is this a reasonable application of the Don't Repeat Yourself principle?
The code is as follows:
public class ArgumentValidator
{
public static void ValidateArguments(params object[] methodArguments)
{
for (int i = 0; i < methodArguments.Length; i++)
{
if (methodArguments[i] == null)
{
ParameterInfo param = GetCallingMethodParameterInfo(i);
// Raise the exception on behalf of the caller
// I include the parameter name because that's conventional for this argument in C#
throw new ArgumentNullException(param.Name);
}
else if (methodArguments[i].GetType() == typeof(string))
{
string argCast = methodArguments[i] as string;
if (!argCast.Trim().Any())
{
ParameterInfo param = GetCallingMethodParameterInfo(i);
throw new ArgumentException(param.Name + " is empty or consists only of whitespace");
}
}
}
}
/// <summary>
/// Get <see cref="ParameterInfo"/> for a specific parameter from the calling method
/// </summary>
/// <param name="index">Index of the argument</param>
/// <returns><see cref="ParameterInfo"/> for the parameter in question</returns>
private static ParameterInfo GetCallingMethodParameterInfo(int index)
{
StackTrace trace = new StackTrace();
// Get the method that called the original method
MethodBase info = trace.GetFrame(2).GetMethod();
// Get information on the parameter that is null so we can add its name to the exception
ParameterInfo param = info.GetParameters()[index];
return param;
}
As an example of how this is supposed to be used, here's my unit test for this method (which obviously passes):
[TestMethod]
public void ArgumentValidatorTests()
{
// Should run without exception
RunTest("a", "b", "c");
MethodInfo info = GetType().GetMethod("RunTest", BindingFlags.NonPublic | BindingFlags.Instance);
bool result = CommonTests.ThrowsException(info, this, typeof(ArgumentException), new[] { "abc", "def", "" });
Assert.IsTrue(result, "Didn't throw an argument exception with an empty string");
result = CommonTests.ThrowsException(info, this, typeof(ArgumentException), new[] { "abc", "def", " " });
Assert.IsTrue(result, "Didn't throw an exception with a string that consisted only of whitespace");
result = CommonTests.ThrowsException(info, this, typeof(ArgumentNullException), new[] { "abc", null, "def" });
Assert.IsTrue(result);
}
private void RunTest(string a, string b, string c)
{
ArgumentValidator.ValidateArguments(a, b, c);
}
The ThrowsException method is as follows:
internal class CommonTests
{
/// <summary>
/// See if a particular method throws the exception we expected
/// </summary>
/// <param name="method">Method to invoke</param>
/// <param name="toRunOn">Object to invoke the method on</param>
/// <param name="expectedExceptionType">Expected exception</param>
/// <param name="args">Arguments for the method</param>
/// <returns><c>true</c> if the method in question</returns>
internal static bool ThrowsException(MethodInfo method, object toRunOn, Type expectedExceptionType, params object[] args)
{
try
{
method.Invoke(toRunOn, args);
}
catch (TargetInvocationException e)
{
return e.InnerException.GetType() == expectedExceptionType;
}
// Didn't throw an exception at all
return false;
}
}
StackTrace
. Really you can only rely on StackTrace working "correctly" in Debug mode, but can you imagine what would happen if you tried to call the method when something up the stack has been inlined in release? Those are hard-to-track-down bugs, "works in debug, breaks in release"... I'm also not sure why you don't useString.IsNullOrWhiteSpace()
instead ofTrim().Any()
? \$\endgroup\$