1
\$\begingroup\$

I'm having a WCF Rest service which does input parameter validation by using the IParameterInspector, before calling the actual service method. I have Implemented this functionality in my own way, but i have feeling that there may be some optimized way to implement this functionality. That is why I came here for a review. Here is my code snippet.

public class ValidationParameterInspectorAttribute : Attribute, IParameterInspector, IOperationBehavior
{
    public object BeforeCall(string operationName, object[] inputs)
    {
        //ValidateMethodParameters class have this static "StartValidatingParameters" method.
        ValidateMethodParameters.StartValidatingParameters(operationName, inputs);
        return null;
    }

    public void AfterCall(string operationName, object[] outputs, object returnValue, object correlationState)
    {
    }

    ...............
}

When client consume rest API, WCF first calls this BeforeCall() method which contain logic of validating parameters. I have made seperate class named - ValidateMethodParameters which contain static method to validate params and throws appropriate exception. Here is the code snippet.

public class ValidateMethodParameters
{
    internal static void StartValidatingParameters(string operationName, object[] inputs)
    {
        switch (operationName)
        {
            case "RejectConsultById":
                Validate_AcceptRejectConsultById(inputs);
                break;

            case "SaveResetPasswordRequest":
                Validate_SaveResetPasswordRequest(inputs);
                break;

            case "CloseReferralById":
            case "CompleteReferralById":
                Validate_CloseOrComplateReferralById(operationName, inputs);
                break;

            //...  Other method ....
        }
    }

    private static void Validate_AcceptRejectConsultById(object[] inputs)
    {
        var methodParams = inputs.FirstOrDefault() as ConsultAcceptRejectViewModel;

        if (methodParams != null &&
            (methodParams.iConsultID <= 0))
        {
            ErrorLogger.LogErrorMessageToElmah(ThrowRangeException(operationName, "iConsultID"));
            ThrowFaultException();  // This stops the execution of Rest API and return to client
        }
        else if (methodParams.iUserID <= 0)
        {
            ErrorLogger.LogErrorMessageToElmah(ThrowRangeException(operationName, "iUserID"));
            ThrowFaultException();  // This stops the execution of Rest API and return to client
        }
        else if (methodParams.iPatientID <= 0)
        {
            ErrorLogger.LogErrorMessageToElmah(ThrowRangeException(operationName, "iPatientID"));
            ThrowFaultException();
        }
        else if (methodParams.iSenderPhysicianID <= 0)
        {
            ErrorLogger.LogErrorMessageToElmah(ThrowRangeException(operationName, "iSenderPhysicianID"));
            ThrowFaultException();
        }
    }

    private static void Validate_SaveResetPasswordRequest(object[] inputs)
    {
        var methodParams = inputs.FirstOrDefault() as ResetPassword;
        if (string.IsNullOrEmpty(methodParams.cEmailID))
        {
            ErrorLogger.LogErrorMessageToElmah(ThrowRequiredValueException("SaveResetPasswordRequest", "cEmailID"));
            ThrowFaultException();
        }
    }

    // Throw exception for elmah if parameter value is not satisfied certain limit
    private static Exception ThrowRangeException(string methodName, string parameterName)
    {
        return new ArgumentException(string.Format("API Name: {0}   Parameter cannot be less than zero.", methodName), parameterName);
    }
    // Throw exception for elmah if parameter value is blank or null
    private static Exception ThrowRequiredValueException(string methodName, string parameterName)
    {
        return new ArgumentNullException(parameterName, string.Format("API Name: {0}   Parameter cannot be blank or null.", methodName));
    }

    private static void ThrowFaultException()
    {
        WebOperationContext.Current.OutgoingResponse.ContentType = "application/json";
        var wfc = new WebFaultException<Response>(new Response(false, Convert.ToString(System.Configuration.ConfigurationManager.AppSettings["ClientErrorMsg"])), System.Net.HttpStatusCode.OK);
        throw wfc;
    }
}

So as you have seen in my Validate_AcceptRejectConsultById method, i have write if - else condition to validate value. Any comments and suggestions are most welcome.

Sorry, if you find this question very silly.

\$\endgroup\$
2
\$\begingroup\$

These validations are flawed.

Validate_AcceptRejectConsultById()

If either the passed-in argument object[] inputs is null or they aren't of type ConsultAcceptRejectViewModel this method throws if iConsultID >0 because you are checking in the second if condition for methodParams.iUserID <= 0.

Validate_SaveResetPasswordRequest()

If the passed in argument object[] inputs is null or they aren't of type ResetPassword this method throws at the if condition.

If you are using a soft cast with as you should always check for null. If you are by any chance using C# 6.0 you can use the null-conditional operator ? like so

private static void Validate_SaveResetPasswordRequest(object[] inputs)
{
    var methodParams = inputs.FirstOrDefault() as ResetPassword;
    if (string.IsNullOrEmpty(methodParams?.cEmailID))
    {
        ErrorLogger.LogErrorMessageToElmah(ThrowRequiredValueException("SaveResetPasswordRequest", "cEmailID"));
        ThrowFaultException();
    }
}

methodParams?.cEmailID means, if methodParams != null access the property cEmailID.

But as a guess, you want to throw some kind of exception if methodParams is null.


var methodParams = inputs.FirstOrDefault() as ResetPassword;  

You are using FirstOrDefault() so I would expect the variable which holds the value will be named having a singular meaning, so methodParams either should be methodParam or methodParameter.


internal static void StartValidatingParameters(string operationName, object[] inputs)
{
    switch (operationName)
    {
        case "RejectConsultById":
            Validate_AcceptRejectConsultById(inputs);
            break;

        case "SaveResetPasswordRequest":
            Validate_SaveResetPasswordRequest(inputs);
            break;

        case "CloseReferralById":
        case "CompleteReferralById":
            Validate_CloseOrComplateReferralById(operationName, inputs);
            break;

        //...  Other method ....
    }
}  

these strings you use here to compare against the operationName should be extracted to private static readonly strings.

Also, if the operationName is "CompleteReferralById" I would expect the validation method to be named Validate_CloseOrCompleteReferralById

Clarification based on comment:

Sample for extracting string to private static readonly variable

private static readonly string RejectConsultById = "RejectConsultById";
....  

internal static void StartValidatingParameters(string operationName, object[] inputs)
{
    switch (operationName)
    {
        case RejectConsultById:
            Validate_AcceptRejectConsultById(inputs);
            break;  

        ....  

Although you say, null values can never be happen, you should always expect them. The class and the method are public so there is a chance that it will be accessed. If another developer takes over this project and allows null values, the application will crash.

\$\endgroup\$
  • \$\begingroup\$ As you said - passed in argument object[] inputs is null. Well in my case it'll never be null and that's why i have not checked for null in Validate_SaveResetPasswordRequest method. Okay other thing you said is - "strings you use here to compare against the operationName should be extracted to private static readonly strings" (For StartValidatingParameters method) - well i have not got this point. Can you give some exa. ?? Now apart from all of these i still have one question - Do you think that this approach is valid ? Is there any optimized way exists ??? Thanks for your suggestion. \$\endgroup\$ – Krishnraj Rana Oct 5 '15 at 11:20
  • \$\begingroup\$ Updated my answer. Why do you think that approach is not valid ? It looks ok to me. \$\endgroup\$ – Heslacher Oct 5 '15 at 11:27
  • \$\begingroup\$ What is the benefit of using private static readonly string over directly compare string value in switch case ?? Regarding null check you are absolutely correct, let me think on that. And regarding this approach, i feel that i might use reflection by adding attribute to my viewmodel property which is used here for validation, but then again i feel that reflection might hurt app performance. That's why i'm asking you again and again for alternate approach. Thanks again for your reply. \$\endgroup\$ – Krishnraj Rana Oct 5 '15 at 11:46
  • 1
    \$\begingroup\$ Benefit against comparing to string, is simply that if you somewhere in this class need to use them again, you have one centralies location if any changes needs to be done. There won't be any typos introduced by copy and paste. Its just good practice. \$\endgroup\$ – Heslacher Oct 5 '15 at 11:48

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.