6
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ I would just suggest using a Range[2,7] attribute for this property. Validate an entire model at once instead of individually for each property. If an "invalid" value is allowed, make it null instead of -1, which other may find confusing. \$\endgroup\$
    – Brad M
    Commented Sep 14, 2015 at 14:14
  • \$\begingroup\$ I looked for a couple of examples on using [Range] but I still don't fully understand it. Say I set the range attribute like so: [Range(1, 8, ErrorMessage = "Helpful error message.")] What happens when I set an invalid value? Do I get the error message to console or elsewhere? Does this stop the code from continuing to execute? Can I somehow customize this error message portion? \$\endgroup\$
    – endrnc
    Commented Sep 14, 2015 at 14:44
  • \$\begingroup\$ The validation attributes don't really do anything by themselves - they serve as metadata. There are different ways to validate the model itself. For example, if receiving this model as a parameter for an API endpoint, you can create an actionFilter that does something like actionContext.ModelState.IsValid. This is a built-in property that validates the entire model and generates a response with any/all appropriate error messages. \$\endgroup\$
    – Brad M
    Commented Sep 14, 2015 at 14:52

4 Answers 4

11
\$\begingroup\$

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

\$\endgroup\$
3
  • \$\begingroup\$ I think you're absolutely right. int is not the best type I could use here, solely because it can handle so many values. The enumeration provides a simple solution for what I want to do with the code, and becomes extensible for future development. I also see dcastro's solution of throwing an exception for invalid arguments in this solution, which I now believe is necessary. Thanks! \$\endgroup\$
    – endrnc
    Commented Sep 14, 2015 at 17:50
  • \$\begingroup\$ @endrnc The enumeration can hold just as many values as an int as well. In .NET, enum types are basically int (or a different type, if you specify it) in disguise. \$\endgroup\$ Commented Sep 14, 2015 at 18:59
  • \$\begingroup\$ It's true that the enum can hold just as many types, but at runtime it can only reference as many as you have explicitly stated before compiling. This is essentially behavior I'm looking for. Unless there's some method to adding items to an enumeration that I haven't heard of before (which I would love to read/learn about if there is!) \$\endgroup\$
    – endrnc
    Commented Sep 14, 2015 at 19:36
3
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ I see how it brings about unexpected behavior, which I guess is what crawled under my skin in the first place. How would you recommend I separate the validation from the setting? I'd like to not check the validation everywhere I use this value, and instead check once when I set the value. \$\endgroup\$
    – endrnc
    Commented Sep 14, 2015 at 14:46
  • \$\begingroup\$ Hmm, having only 6 possible values makes me think "int" is not the most suitable type for the property, could be even "primitive obsession smell" (serch on Google for this subject). On the other hand, int might be the suitable type but in this case are you really sure that the valid Config property's value could be only 2-7 for all "consumers"? If you are not sure what future modification requests might bring, it'd better idea to separate concerns regarding data and validation - in the real world it's usually someone other's responsibility to validate values. \$\endgroup\$ Commented Sep 14, 2015 at 15:50
  • \$\begingroup\$ Check also stackoverflow.com/questions/6127290/… \$\endgroup\$ Commented Sep 14, 2015 at 15:50
3
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ The quote says "get accessors", you're talking about a setter. \$\endgroup\$ Commented Sep 14, 2015 at 14:07
  • \$\begingroup\$ @BenAaronson That's a good point. I'll update my answer \$\endgroup\$
    – dcastro
    Commented Sep 14, 2015 at 14:08
  • \$\begingroup\$ In other words, throwing an exception from the setter where it's currently setting _config to -1 right? What's the downside to doing this during a set operation if any? \$\endgroup\$
    – endrnc
    Commented Sep 14, 2015 at 14:52
  • \$\begingroup\$ @endrnc That's correct. I don't know of any downside - I see that approach all the time. \$\endgroup\$
    – dcastro
    Commented Sep 14, 2015 at 15:09
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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