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;
}
}
1 Answer 1
Repetition & Performance
You can save some time and improve the performance by not getting the stack for each parameter separately. As a matter of fact you can jump right to the right frame. The GetCallingMethodParameterInfo
is completely redundant. You can have everything at once by using LINQ.
- Get the stack trace by skipping the first frame (you don't need it).
- Get the calling method parameters.
Zip
the arguments with the parameters.- Use a
foreach
to loop over both the arguments and their corresponsing parameter info at the same time. - Check the
string
first not thenull
because thestring
requires special handling. - It might be a good idea to use this only in debug mode so the
Conditional
attribute is really handy here.Indicates to compilers that a method call or attribute should be ignored unless a specified conditional compilation symbol is defined.
Example:
[Conditional("DEBUG")]
public static void ValidateArguments(params object[] args)
{
var stackTrace = new StackTrace(skipFrames: 1);
var method = stackTrace.GetFrame(0).GetMethod();
var parameters = method.GetParameters();
var items = args.Zip(parameters, (arg, param) => new
{
Argument = arg,
Parameter = param
});
foreach (var item in items)
{
if (item.Parameter.ParameterType == typeof(string))
{
var value = (string)item.Argument;
if (string.IsNullOrWhiteSpace(value))
{
throw new ArgumentException(
paramName: item.Parameter.Name,
message: $"{method.Name}'s argument \"{item.Parameter.Name}\" must not be null or white-space.");
}
}
if (item.Argument == null)
{
throw new ArgumentNullException(
paramName: item.Parameter.Name,
message: $"{method.Name}'s argument \"{item.Parameter.Name}\" must not be null");
}
}
}
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\$