Input Data Validator (good practice)

Question

I work at an application that receives data from user and I try to write a validator function for it, but I'm not sure if this is the correct way to proceed.

Example: The user will input a number (as string, don't ask why I don't use an int, let it be a string), let's say "103", and I'will use this number inside a function, but first, at the beginning of that function I call a validator function:

private bool ValidateCommandCode(string code)
{
    bool isValid = false;

    byte commandByte = new byte();

    if (byte.TryParse(code, out commandByte))
    {
        isValid = true;
    }
    else
    {
        Log.Error($"Command number {CommandCode} for the request is not valid!");
        isValid = false;
    }

    return isValid;
}

private async void MainFunction()
{
    if (ValidateCommandCode(CommandCode) == false)
        return;

    // ... do the magic with the CommandCode ...

}

in the same manner, I want to validate another field filled by the user: e.g of data: 000A000B

private bool ValidateRequestData(string data)
        {
            bool isValid = false;
            
            if (!String.IsNullOrEmpty(Payload) && !String.IsNullOrWhiteSpace(Payload))
            {
                if (Payload.Trim().Replace(" ", "").Length % 2 != 0)
                {
                    Log.Error($"Payload (Data) {Payload} for the request doesn't have an even number of bytes!");
                    isValid = false;

                }
                else
                {
                    isValid = true;
                }
            }
            else
            {
                isValid = true;
            }

            return isValid;
        }

Is this a good way to proceed? Aren't so many flags "isValid" too confusing?

Answer 1

First off, in here

if (!String.IsNullOrEmpty(Payload) && !String.IsNullOrWhiteSpace(Payload))

You only need to do

if(!String.IsNullOrWhiteSpace(Payload))

There is a lot of nesting going on and you can simplify your code to make it easier to read by doing this instead

private static bool ValidateRequestData(string data, string payload)
{
    if (String.IsNullOrWhiteSpace(data)) return false; // or throw
    if (String.IsNullOrWhiteSpace(payload)) return false;

    if (payload.Trim().Replace(" ", "").Length % 2 != 0)
    {
        //Log.Error(...);
        return false;
    }
    
    return true
}

If you also notice, we are passing in the payload as an argument and also make this a static method. This is generally a safer design as the state of your member Payload is likely not guaranteed to remain the same (or at least it's not obvious).

I would also recommend changing the method name from ValidateRequestData() to IsValidRequestData().

---

As an extra. We can also clean up you other method

private static bool ValidateCommandCode(string code)
{
    if (!byte.TryParse(code, out byte commandByte))
    {
        //Log.Error(...);
        return false;
    }

    return true;
}

Although, keep in mind the Parse could probably have happened somewhere else. It doesn't seem very efficient to do a parse only for the purpose of validation and not making any use of the result.

Answer 2

You can simplify it further like this :

private bool ValidateRequestData(string data)
{
    return !string.IsNullOrWhiteSpace(Payload) && Payload.Trim().Replace(" ", "").Length % 2 != 0;
}

private bool ValidateCommandCode(string code)
{
    return byte.TryParse(code, out byte commandByte);
}

and for the logging, I would suggest you keep it outside the validation itself, so you know which part of your code has logged it, and in this way, you can control the logging to which part of your code should log the validation.

private async void MainFunction()
{
    if(!ValidateCommandCode(CommandCode))
    {
        Log.Error($"Command number {CommandCode} for the request is not valid!");       
    }
    else
    {
        // ... do the magic with the CommandCode ...    
    }
}

Answer 3

You didn't tell us what kind of application you maintain but when it's something with a UI then you shouldn't need this kind of validation at all. Instead you should use proper masks for the input fields and prevent users from entering wrong characters entriely.