10

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;                
        }
    }
5
  • 47
    I don't have time for a full answer, but making a setter prompt for user input is a really bad idea in terms of trying to write idiomatic C# code... Commented Sep 28, 2011 at 17:25
  • @JonSkeet sorry for the naive question but what is idiomatic C# code? Commented Sep 28, 2011 at 17:36
  • Trapping the user in a field is bad UI design. Commented Sep 28, 2011 at 17:38
  • 7
    @JamesAlexander idiomatic code [in any language] is code that follows generally or widely accepted patterns of usage. It's code that "looks like" other code in that language. Doing console iteractions in a property setter is NOT something that is commonly done in C#. Not only is it not idiomatic, its just not a good idea period. Commented Sep 28, 2011 at 17:39
  • 4
    Just to be clear, it's not just that it is a property setter that makes this a bad idea. Your Person class shouldn't be doing IO period! Changing this to a method won't make the design any better. IO should be handled by a dedicated class (like your Program class or something better) - the principle that you're violating here is called Separation of Concerns: en.wikipedia.org/wiki/Separation_of_concerns. Commented Sep 29, 2011 at 12:49

9 Answers 9

20

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.
Sign up to request clarification or add additional context in comments.

1 Comment

I modified your code because my visual studio don't understand regex part
6

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

Comments

3

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).

Comments

2

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

Comments

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

Comments

1

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.

Comments

1

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

3 Comments

You do know that a property is for the most part just syntactic sugar for getter/setter methods. Handling the validation outside the setter is a good idea, but there's no need to go away from properties entirely.
I am aware, but imo just seeing a list of properties tells you nothing about what can be performed on a model (assuming Person is some sort of domain model), but seeing a bunch of verbs (methods) tells you something.
Also I don't normally expect properties to throw exceptions when I set their value
1

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,

Comments

0

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
     }

6 Comments

He needs to do the opposite; that should be a continue, as should the earlier instance of break. But this should not exist at all.
a lot of noob programmers ask questions to syntax/logical bugs in their design-flawed code. My answer is helping him with his bug (which is the answer to his question), not redesign his code (tho that would be more help), and a downvote for that is silly..
@Shredder - Agreed about the downvote, but its worth taking the time to point out really egregious issues.
@RitchMelton I tried pointing that out by saying "aside from what Skeet said", since it was already mentioned >< Guess I should have reiterated more
@Shredder : I didn't downvote; the issue I pointed out was the reversal of the logic that you had (and have since corrected).
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.