13
\$\begingroup\$

I'm at uni and working on an assignment in c# that takes some user input (from they keyboard via console).

The input validation required:

  • numbers must be within range (range will vary depending on which menu they're in)
  • strings must not be empty/null
  • string must be no longer than 30 characters in length

So I've chosen to write just one method (will be a static method of a 'validator' class) that can handle validation for both and am unsure if this is a good idea - having a method that essentially does more than one thing.

The method:

public static bool isValid(string input, string type, int min = 0, int max = 0)
  {
     if (String.IsNullOrEmpty(input))
        return false;
     else
     {
        switch (type.ToLower())
        {
           case "integer":
              int i;
              if (Int32.TryParse(input, out i))
                 return ((i >= min) && (i <= max));
              else
                 return false;
           case "string":
              return input.Length < charLimit; // charLimit defined as 30 above
           default:
              return false;
        }
     }
  }

And a sample call:

if(isValid(userInput, "integer", 0, 5)
{
   // do something awesome
}

I've run some tests to check the edge cases and passing in empty strings etc and it passed them, but for some reason this feels kind of wrong.

Should I separate string and integer validation into separate methods?

\$\endgroup\$
  • \$\begingroup\$ I only selected an answer over @Heslacher as it built upon it, as a student both were equally valuable, thanks again \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 12:45
10
\$\begingroup\$

This is on top of what @Heslacher said.

Flat is good

Since the if branch of the first condition returns, you can eliminate the else branch to reduce the indent level of the rest of the function. Flatter code is often easier to read.

if (String.IsNullOrEmpty(input))
{
    return false;
}

switch (type.ToLower())
{
    case "integer":
        return IsValidInteger(input);
    case "string":
        return IsValidString(input);
    default:
        return false;
}

Naming

i is typically used in counting loops, so it's not a great name for a number. num would be better.

Ordering of terms in a condition

Instead of this:

return ((i >= min) && (i <= max));

It's generally easier to read when the terms are in a consistent numeric order:

return ((min <= i) && (i <= max));

I simply flipped the sign of the first term, and now the values read from low to high.

The parentheses are also redundant, you can simplify to:

return min <= i && i <= max;
\$\endgroup\$
  • \$\begingroup\$ Both of these answers are really helpful, the result looks much cleaner and easier to read. Thanks \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 7:17
  • \$\begingroup\$ I believe SHOUT_CASE is only used in C# when you want to match an existing scheme (when you copy constants from Windows API, for example). In other cases you normally use PascalCase for constants (same goes for static readonly fields). stackoverflow.com/questions/242534/… \$\endgroup\$ – Nikita B Jun 17 '16 at 9:48
  • \$\begingroup\$ @NikitaB fair enough, I dropped that point, thanks \$\endgroup\$ – Stop ongoing harm to Monica Jun 17 '16 at 11:33
  • \$\begingroup\$ The statement of min <= i && i <= max being easier to read is quite subjective. Many of us prefer i >= min && i <= max, re: "I is greater than the min value and less than the max value". \$\endgroup\$ – Metro Smurf Jun 17 '16 at 23:08
  • \$\begingroup\$ @MetroSmurf for what it's worth, it's recommended in Code Complete, and the writing style is just one step away from the handy min <= ... <= max operator supported by some languages \$\endgroup\$ – Stop ongoing harm to Monica Jun 18 '16 at 4:17
11
\$\begingroup\$

You are clearly violating the Single Responsibility Principle (SRP) because the method is doing two things. You should use two methods where each validates one type only.

You will see also that for this task you gain the advantage that the methods will be smaller and easier to read and understand.

A small side note: In .NET methods are named using PascalCase, so isValid should be IsValid.

Don't omit braces {} although they might be optional. By always using them your code will become less error prone.


Another quick remark about the method in question. You have defined optional parameters min = 0 and max = 0. I would expect if parameters are optional, that they could return true for a bool method. Using this method without specifying min and max will result in false which is unexpected.

Integrating the said points into extension methods would look like so

public static bool IsInRange(this int value, int min, int max)
{
    return min <= value && value <= max;
}  

public static bool HasValidLength(this string value, int maximumLength)
{
    if (value == null) { throw new ArgumentNullException("value"); } 

    return value.Length < maximumLength;
}  

which could then be called through this methods

public static bool IsValidInteger(this string value, int min = int.MinValue, int max = int.MaxValue)
{
   int v = 0;
   return int.TryParse(value, out v) && v.IsInRange(min, max);
}  

public static bool IsValidString(this string value, int maximumLength = 30)
{
    return value != null && value.HasValidLength(maximumLength);
}

like e.g

if(userInput.IsValidInteger(0, 5))
{
   // do something awesome
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! Much appreciated, I have read of the single responsibility principle and now it'll be in my mind when writing methods \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 7:15
  • \$\begingroup\$ Ok so how about a 'Validator' class with a public static method 'IsValid' which in turn would call a private method 'IsValidInteger' etc? \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 12:37
  • \$\begingroup\$ As it is already static I would just make extension methods, one for int and one for string both being public. I can post an example on monday if you still need it. I am already in weekend and only on the phone. \$\endgroup\$ – Heslacher Jun 17 '16 at 13:12
  • \$\begingroup\$ All good, you have given me more than enough to go on, I'll start to incorporate this into my solution. Though if Monday comes and you find time please do post an example, I would appreciate it, cheers \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 13:58
4
\$\begingroup\$

On top of all other answers... Your current code can be made even simpler. There is no need for the string type parameter. Just try to parse it as int and if this doesn't work assume it's a string. After all you need only one validation method which is IsInRange that you can use for both numbers and strings.

const int charLimit = 30;

public static bool IsValid(string input, int min = 0, int max = 0)
{
    if (string.IsNullOrEmpty(input))
    {
        return false;
    }

    var i = 0;
    if (Int32.TryParse(input, out i))
    {
        return IsInRange(i, min, max);
    }

    return IsInRange(input.Length, 0, charLimit);
}

public static bool IsInRange(int i, int min, int max)
{
    return ((i >= min) && (i <= max));
}
\$\endgroup\$
  • \$\begingroup\$ This is a good thought, but it may or may not be applicable. If the field is supposed to be a number, then "abc" is not valid, even though it passes the string validation. Good reuse of IsInRange(), though. \$\endgroup\$ – Bobson Jun 17 '16 at 17:12
  • \$\begingroup\$ @Bobson sure, this might all be true however we don't know the exact requirements so according to the YAGNI rule this is the least we need looking at the sample code ;-) \$\endgroup\$ – t3chb0t Jun 17 '16 at 18:13
  • \$\begingroup\$ You're right. The OP is unclear, and now that you point it out I can read it both ways. \$\endgroup\$ – Bobson Jun 19 '16 at 2:56
3
\$\begingroup\$

Heslacher is correct in that you should split this into two separate methods. That said, sometimes doing that isn't an option, so I want to point out an alternative to passing the "magic values" of "string" and "integer" into your function.

When you have a function argument that only takes a very limited number of values, you're usually better off replacing it with an enum. That enforces the limitation when you're writing code, and helps avoid typos and other subtle gotchas.

public enum ValidationType
{
   String,
   Integer,
}

public static bool isValid(string input, ValidationType type, int min = 0, int max = 0)
{
  // ... 
  switch (type)
  {
     case ValidationType.Integer:
          // ...
     case ValidationType.String:
          // ...
     default:
          throw new ArgumentException(type);
  }
}

This removes the need to worry about casing, lets IntelliSense prompt you for valid values, and throwing an exception on an unexpected value ensures that you catch it during testing the moment it occurs, instead of the subtle "hey, this just keeps failing to validate" of always returning false.

\$\endgroup\$
  • \$\begingroup\$ See my answer on dealing with the validation use "Type" \$\endgroup\$ – d347hm4n Jun 17 '16 at 13:55
0
\$\begingroup\$

In conjunction with the answers provided above about the violation of the single point of responsibility and keeping the code as "Flat" as possible. One of my pet hates is passing in a string of the type your are dealing with.

Use the .net provided "Type" class and then evaluate it against type you wish to deal with at the appropriate time.

private static readonly int charLimit = 30;

    public static bool IsValid(string input, Type type, int min = 0, int max = 0)
    {
        if (IsNullOrEmpty(input))
        {
            return false;
        }

        if (type == typeof(int))
        {
            int i;
            if (int.TryParse(input, out i))
            {
                return (i >= min) && (i <= max);
            }

            return false;
        }
        if (type == typeof(string))
        {
            return input.Length < charLimit;
        }

        return false;
    }
\$\endgroup\$
  • \$\begingroup\$ I need to investigate the Type class as I thought all input coming through keyboard via console in c# is a string until parsed, hmm \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 14:09
  • \$\begingroup\$ You are correct - everything from the console is as a string. But clearly you are in various places asking for either a string or an integer. In those places, you mark the call IsValid(input, typeof(int), 0, 10) or IsValid(input, typeof(string)); \$\endgroup\$ – d347hm4n Jun 17 '16 at 14:16
  • \$\begingroup\$ Ah ok now I understand, I'm new to C# so this is all great help \$\endgroup\$ – Mathew Harrington Jun 17 '16 at 14:24
  • \$\begingroup\$ Your going great guns. Nicely laid out question and insightful questions. +1 internet points for you sir. \$\endgroup\$ – d347hm4n Jun 17 '16 at 14:36
  • \$\begingroup\$ Ugh. Passing a Type as an argument is ugly. If you want that functionality, use generics. Also, this is only a marginal improvement over passing a string value. It prevents typos, but doesn't solve the problem of passing typeof(uint), typeof(decimal), or typeof(MyCustomClass), all of which are now valid calls and won't produce the expected behavior. \$\endgroup\$ – Bobson Jun 17 '16 at 16:14

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.