10

I'm attempting to resolve the following exercise:

You need to create a class named Product that represents a product. The class has a single property named Name. Users of the Product class should be able to get and set the value of the Name property. However, any attempt to set the value of Name to an empty string or a null value should raise an exception. Also, users of the Product class should not be able to access any other data members of the Product class. How will you create such a class?

I have created the following code but for some reason it does not throw the exception when the string is invalid:

class Program
{
    static void Main(string[] args)
    {
        Product newProduct = new Product();
        Console.WriteLine("Enter Product name:");
        newProduct.Name = null; //Console.ReadLine();
        Console.WriteLine("Product name is : {0}", newProduct.Name);
        Console.ReadLine();
    }
}

class Product
{
    private string name;
    public string Name
    {
        get
        {
            return this.name;
        }
        set
        {
            if (Name != String.Empty || Name != null)
            {
                name = value;
            }
            else
            {
                throw new ArgumentException("Name cannot be null or empty string", "Name");
            }
        }
    }
}

Is the exception not thrown because I do not have try-catch statement? I was also wondering is it possible to have only catch statement without try statement?

Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
Pawel
  • 309
  • 1
  • 3
  • 10
  • You are checking previous value (which is valid). Use `value` instead of `Name` in `if` to validate new value. Or just try to set `Name` inside `Main` twice ;) – Sinatr Sep 17 '15 at 13:09
  • 2
    You cannot catch without try. Otherwise, how would it know what code that it is supposed to catch from? Even if you don't have a try/catch, exceptions still get thrown. You're just not reaching the code. Learn to use your IDE's debugging tools to step through the code line by line as it executes. I think you'll spot the issue fairly quickly that way. – mason Sep 17 '15 at 13:11
  • 3
    You're using an `or` in your if statement. If `Name == null`, then `Name != String.Empty` and that part passes. Use `&&`. – Jake Sep 17 '15 at 13:11
  • possible duplicate of [How can I check a C# variable is an empty string "" or null?](http://stackoverflow.com/questions/8224700/how-can-i-check-a-c-sharp-variable-is-an-empty-string-or-null) – Liam Sep 17 '15 at 13:33

3 Answers3

12

Use String.IsNullOrEmpty Method (String). Change your set like this:

set
{
      if (!string.IsNullOrEmpty(value))
      {
            name = value;
      }
      else
      {
            throw new ArgumentException("Name cannot be null or empty string", "value");
      }
}

Also you can use String.IsNullOrWhiteSpace Method (String) that Indicates whether a specified string is null, empty, or consists only of white-space characters.

Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
7

Your if state is wrong. Let's do a truth table:

if (value != String.Empty || value != null)

Name = null   True Or False  = True 
Name = "name" True Or True = True
Name = ""     False Or True = True 

Your if statement is always true!

I would re-write it thus:

if (value == String.Empty || value == null)
{
     throw new ArgumentException("Name cannot be null or empty string", "Name");  
}
else
{
     name = value;
}

you could just change the Or to and AND but I think the above reads better (the below has an unnecessary double negative):

if (value != String.Empty && value != null)
{
   name = value;
}
else
{
    throw new ArgumentException("Name cannot be null or empty string", "value");
}

As Dmitry Bychenko says, I didn't notice you were not testing for value. In getters you should use the value property. Not the name of your property


The second parameter (again pointed out by Dmitry Bychenko) in your exception should be:

The name of the parameter that caused the current exception.

MSDN

which in your case is the string "value":

throw new ArgumentException("Name cannot be null or empty string", "value");
Liam
  • 27,717
  • 28
  • 128
  • 190
  • Yes but syntactically it reads better to switch the statements. *Is the string empty **or** null then it's wrong*. – Liam Sep 17 '15 at 13:16
  • errh you should always compare against `!= null` first. It's just 'good practice' so you'll never do something like `if(x.Prop1 == "test" && x != null)` – Steffen Winkler Sep 17 '15 at 13:19
  • But this is a value type....that isn't relevant here. That only applies to reference types. – Liam Sep 17 '15 at 13:23
  • 2
    @Liam: I'm very sorry for my impoliteness, but the incorrect comparison actually ruined all the work; now it's OK , so +1 from me (side note: update, please, `ArgumentException` as well: "value" in the text and "value" as the argument name) – Dmitry Bychenko Sep 17 '15 at 13:56
  • this answer my question, but indeed I need to check the value not a Name, nonetheless it answered my question and explain why. I can also see it was edited. Thank you @Liam! – Pawel Sep 17 '15 at 13:57
  • Sorry, @DmitryBychenko, not sure I follow you? Do you mean the second value in the Exception should be value (`throw new ArgumentException("Name cannot be null or empty string", value);`). The one used seems [a valid override](https://msdn.microsoft.com/en-us/library/sxykka64(v=vs.110).aspx) – Liam Sep 17 '15 at 14:01
  • *"The name of the parameter that caused the current exception."* – Liam Sep 17 '15 at 14:02
  • 1
    @Liam: it should be `"value"` not `value`'s value but a string "value". It's parameter "value" provided by "set" causes the exception. – Dmitry Bychenko Sep 17 '15 at 14:02
3

If you want different exceptions on null and on empty string (often null means that something is totally wrong, when empty string is just a format error):

public string Name {
  get {
    return name;
  }
  set {
    if (null == value)
      throw new AgrumentNullException("value");
    else if (String.Equals(value, ""))
      throw new AgrumentException("Empty values are not allowed.", "value");

    name = value; 
  }
}

In case you don't want to distiguish them:

public string Name {
  get {
    return name;
  }
  set {
    if (String.IsNullOrEmpty(value))
      throw new AgrumentException("Null or empty values are not allowed.", "value");

    name = value; 
  }
}

Note, that in both cases it's value that you have to test, not a property Name. In your original code the name's (and so Name as well) initial value is null and you'll get exception whatever you try to set.

Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
  • this is only a simple exercise and I believe I can check for both null or empty string in the same exception, but it is a valid point you made and I will keep that in mind. Thanks. – Pawel Sep 17 '15 at 14:09