Checking the validation of a field through the Property mutator method `set`

Question

I'm newish to C#, being mainly Java oriented. I have a field _config which needs to be a value from 1 to 8, and I have this code which checks to make sure I can only set _config to 1-8. If the value given for set is "invalid", _config becomes -1.

public int Config {
    get
    {
        return _config;
    }

    set
    {
        if (value > 8 || value < 1)
        {
            _config = -1;
        }
        else
        {
            _config = value;
        }
    }
}
private int _config; // the actual field that the property 'Config' refers to.

This feels really weird to me, but it does what I need it to do. I'm also thinking about having the setter throw an exception instead of setting _config to -1 when given an invalid value. Am I using Properties correctly, or have I completely misunderstood the point of them?

Answer 1

No one has yet addressed the fact that you have chosen the wrong tool for the job.

What does Config represent? It looks like you should have chosen an enum where you chose an int.

public enum ConfigurationType
{
    None = 0,
    Type1 = 1,
    Type2 = 2,
    // etc.
}

You can get what ConfigurationType an int corresponds to quite easily:

ConfigurationType configType = (ConfigurationType)someInteger;

Then, you can validate it more fluidly:

public ConfigurationType Config
{
    get
    {
        return _config;
    }
    set
    {
        if (ConfigurationType.IsDefined(typeof(ConfigurationType), value))
        {
            _config = value;
        }
        else
        {
            throw new InvalidEnumArgumentException($"The specified value must be a valid value of the {nameof(ConfigurationType)} enumeration.");
        }
    }
}
private ConfigurationType _config; // the actual field that the property 'Config' refers to.

If you don't have C#6.0, your exception would be more along the lines of:

throw new InvalidEnumArgumentException(string.Format("The specified value must be a valid value of the {0} enumeration.", ConfigurationType.ToString()));

---

You should also consider a refactoring of the name Config, it's non-descriptive. There is likely a better name for such a property/field combination. (Had we more context we would likely be able to suggest better names.)

Answer 2

The setter of the property Config has more than one responsibility, instead of just setting the value, it's also validating it and even changing it's value. The purpose of writing the code is not just that it does what you need to - there will very likely be other users (programmers) of your code. Most of them will expect that setter of the property will simply set its value, and not doing lots of other stuff, but the most dangerous point is changing its state. Separate validation from the setter and use other ways to express invalidity of the property's value.

Answer 3

In a sense, your usage of properties is somewhat incorrect.

From the book Effective C# by Bill Wagner:

Properties are methods that can be viewed from the calling code like data. That puts some expectations into your users’ heads. They will see a property access as though it was a data access. After all, that’s what it looks like.

I would consider setting the value to something else other than the input value an unexpected result. In other words, it violates the principle of least surprise:

obj.Config = 10;
Assert.Equal(10, obj.Config); // <-- fails

The usual behaviour is to throw an ArgumentOutOfRangeException.

Answer 4

Please don't do that. This is very confusing for the consumers of the class.

You have two options.

1. Throw exception when setting to invalid value.
2. Automatically use valid value when consumer has set an invalid value. You can optionally log a message somewhere indicating the value system used.

public int Config { get; set; }

private int FinalConfig { get { return Config > 8 || Config < 1 ? -1 : Config; }

In your code, you use FinalConfig everywhere. You can optionally make it public so that consumer can find out which value was used by your code.