4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ The biggest problem here is the use of 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 use String.IsNullOrWhiteSpace() instead of Trim().Any()? \$\endgroup\$ – Ron Beyer Nov 19 '16 at 1:00
  • 1
    \$\begingroup\$ @RonBeyer It would be interesting if you could you show such an example. \$\endgroup\$ – t3chb0t Nov 19 '16 at 5:30
  • 1
    \$\begingroup\$ @RonBeyer Comments are for seeking clarification to the question, and may be deleted. If you have observations about the code, please write an answer instead. \$\endgroup\$ – 200_success Nov 19 '16 at 5:38
  • 1
    \$\begingroup\$ Ron is right AFAIK. Inlined functions will break this code, it was usually fixed (old time we wanted caller name...) with compiler hint to do not inline this code. \$\endgroup\$ – Adriano Repetti Nov 19 '16 at 12:05
3
\$\begingroup\$

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 the null because the string 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");
        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.