Basic user-input string validation

Question

I have been writing a check in a name property of my person abstract class. The problem that i have is that i am trying to implement a piece of code that will not allow the user to leave the field empty or to exceed the name limit with 35characters or in-put a digit but i am stuck with it. If any one can help or suggest me.

    public string Name
    {
        get { return name; }

        set 
        {
            while (true)
            {
                if (value == "" || value.Length > 35)
                {
                    Console.Write("Please Enter Correct Name: ");
                    value = Console.ReadLine();
                    continue;
                }

                foreach (char item in value)
                {                        
                    if (char.IsDigit(item))
                    {
                        Console.Write("Digits Are NotAllowed....\n");
                        Console.Write("Please Enter Correct Name: ");
                        value = Console.ReadLine();
                        break;
                    }
                }
                break;
            }
            name = value;                
        }
    }

Answer 1

Don't do any form of UI or I/O in a property.

public string Name
{
    get { return _name; }

    set 
    {
        if (! Regex.IsMatch(value, @"\w{1-35}"))
           throw new ArgumentException("Name must be 1-35 alfanum");
        _name = value;
    }
}

The exact regular expression is up for discussion but the best practice:

• do not try to list and reject all the patterns you don't like. Too much possibilities.
• accept what you expect (and understand), reject everything else.

Answer 2

From a semantics point of view, a setter is as its name says, a setter! It should be used to set a private/protected field of a class From a testability point of view, your design is very hard to be automatically tested not to say impossible!

This reminds me of a bit of code I worked on sometime ago where a setter is opening a socket and sending stuff over the network! The code should do what it reads, just imagine if someone uses your code, calls your setter and wonders why on earth does his/her application hang (waiting for user input)

The way I see your code more readable and testable is to have a verifer class that ensures the user is entering the right data in the right format. The verifier should take an input stream as data source, this will help you easily test it.

Regards,

Answer 3

public string Name
{
    get { return name; }
    set { name = value; }
}

public static bool IsNameValid(string name) 
{
    if (string.IsNullOrEmpty(name) || name.Length > 35)
    {
        return false;
    }

    foreach (char item in value)
    {                        
        if (!char.IsLetter(item))
        {
            return false;
        }
    }

    return true;

}

Finally a code snippet for reading an user input.

var yourClassInstance = new YourClass();

string input
bool inputRead = false;

while(!inputRead) 
{
    var input = Console.ReadLine();

    inputRead = YourClass.IsNameValid(input);
}

yourClassInstance.Name = inputRead;

Answer 4

The solution for handling this according to your rules are almost obvious but the thing is, it's better not to put the checking and validating logic in the setter method of a property, you can have a separate class for instance and that class does the validation responsibility for you and you can tell it to do that and then use the result appropriately. In that case you are following "Tell, Don't Ask" rule and also "Single Responsibility Principle"

Good Luck

Answer 5

I would create a method for changing the name that contains the validation logic. If you want to check the name is valid, so you don't have to handle the argumentexception do a check first, call IsValidName before calling ChangeName

public class Person
{
    public void ChangeName(string name)
    {
       if (!IsValidName(name))
       {
           throw new ArgumentException(....);
       }
       else
          this.Name = value;
    }

    public bool IsValidName(string name)
    {
        // is the name valid using
    }

    public string Name { get; private set; }
 }

And to use it

 var newName = Console.ReadLine();
 var person = new Person();
 while (!person.IsValidName(newName))
 {
     newName = Console.ReadLine();
 }
 person.ChangeName(newName);

Answer 6

The short answer for this is to loop while the value is not valid:

public string GetName()
{
    String name = String.Null;
    do
    {
        Console.Write("Please Enter Correct Name: ");
        name = Console.ReadLine();
    } while (!ValidateName(name))
}

public bool ValidateName(string name)
{
    //String validation routine
}

That being said, as I'm sure you will see from other answers, change where the Name is given. As a general rule, accessors are really just for "getting" and "setting" quickly what's in a class.

Answer 7

First of all, never ask for Console input inside of a setter. It is a seriously bad practice. Instead, you should throw an Exception from the setter and let the caller handle that however they need:

public string Name
{
    get { return name; }
    set
    {
        if(String.IsNullOrWhiteSpace(value))
            throw new ArgumentException("Name must have a value");
        if(value.Length > 35)
            throw new ArgumentException("Name cannot be longer than 35 characters");
        if(value.Any(c => char.IsDigit(c))
            throw new ArgumentException("Name cannot contain numbers");

        name = value;
    }
}

You can then catch and handle the Exceptions appropriately in the calling code (which, in your case, would involve re-prompting the user for the input).

Answer 8

This sort of validation should be broken up. The setter should only know the various restrictions that it has and throw an exception in the case that an invalid value makes it that far. Do not put user interface code in there.

Try something like this:

public string Name
{
    get { return name; }

    set 
    {
        if (value == "" || value.Length > 35)
        {
            throw new ArgumentException("Invalid name length.");
        }

        foreach (char item in value)
        {                        
            if (char.IsDigit(item))
            {
                throw new ArgumentException("Digits are not allowed.");
            }
        }

        name = value;                
    }
}

Then something like this in your console application:

bool success = false;

while(!success)
{
    try
    {
        Console.WriteLine("Please enter a name:");
        myObject.Name = Console.ReadLine();
        success = true;
    }
    catch(ArgumentException ex)
    {
        Console.WriteLine(ex.Message);
    }
}

Answer 9

Aside from what Mr Skeet said, seems like you should replace this break with a continue in order to validate the new value (like you do in your first length check):

    if (char.IsDigit(item))
    {
         Console.Write("Digits Are NotAllowed....\n");
         Console.Write("Please Enter Correct Name: ");
         value = Console.ReadLine();
         continue;   //here
     }