patterncsharpModerate
Validating integer or string input
Viewed 0 times
inputstringintegervalidating
Problem
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:
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:
And a sample call:
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?
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?
Solution
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
Don't omit braces
Another quick remark about the method in question. You have defined optional parameters
Integrating the said points into extension methods would look like so
which could then be called through this methods
like e.g
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
}Code Snippets
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;
}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);
}if(userInput.IsValidInteger(0, 5))
{
// do something awesome
}Context
StackExchange Code Review Q#132258, answer score: 11
Revisions (0)
No revisions yet.