3
\$\begingroup\$

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?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ "an application that receives data from user" -- what kind of application? In many cases you would be far better off with a relevant NuGet package. Also, why did you tag this "unit testing"? This has got nothing to do with that. \$\endgroup\$
    – BCdotWEB
    Aug 6, 2021 at 12:33
  • 2
    \$\begingroup\$ I removed unit-testing, as you haven't shown us the unit-tests for this code. \$\endgroup\$ Aug 6, 2021 at 12:50

3 Answers 3

5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Great! Thank you for the help! It's a lot cleaner now. \$\endgroup\$
    – Vesa95
    Aug 6, 2021 at 12:06
1
\$\begingroup\$

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 ...    
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ You have the same method signature twice: ValidateRequestData(string). I assume one of them was meant to be named differently? \$\endgroup\$
    – Brian J
    Aug 6, 2021 at 20:52
  • \$\begingroup\$ @BrianJ good catch, yes you're right, they're different, but my clipboard thoughts not ;). fixed it though. \$\endgroup\$
    – iSR5
    Aug 6, 2021 at 21:23
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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