Validating integer or string input

Question

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?

Answer 1

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; 

Answer 2

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 } 

Answer 3

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)); } 

Answer 4

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.

Answer 5

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; }