current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 11 down vote favorite
2

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?

share|improve this question
    
I only selected an answer over @Heslacher as it built upon it, as a student both were equally valuable, thanks again – Mathew Harrington yesterday
up vote 8 down vote accepted

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;
share|improve this answer
    
Both of these answers are really helpful, the result looks much cleaner and easier to read. Thanks – Mathew Harrington yesterday
    
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/… – Nikita B yesterday
    
@NikitaB fair enough, I dropped that point, thanks – janos♦ yesterday
    
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". – Metro Smurf 20 hours ago
    
@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 – janos♦ 15 hours ago
up vote 10 down vote

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.

share|improve this answer
    
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 – Mathew Harrington yesterday
    
Ok so how about a 'Validator' class with a public static method 'IsValid' which in turn would call a private method 'IsValidInteger' etc? – Mathew Harrington yesterday
    
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. – Heslacher yesterday
    
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 – Mathew Harrington yesterday
up vote 2 down vote

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));
}
share|improve this answer
    
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. – Bobson yesterday
    
@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 ;-) – t3chb0t yesterday
up vote 1 down vote

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.

share|improve this answer
    
See my answer on dealing with the validation use "Type" – d347hm4n yesterday
up vote 0 down vote

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;
    }
share|improve this answer
    
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 – Mathew Harrington yesterday
    
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)); – d347hm4n yesterday
    
Ah ok now I understand, I'm new to C# so this is all great help – Mathew Harrington yesterday
    
Your going great guns. Nicely laid out question and insightful questions. +1 internet points for you sir. – d347hm4n yesterday
    
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. – Bobson yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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