Is the property accessor/mutator (getter/setter) the right place to enforce acceptable ranges?

Question

In an app where a port number is set, I want to restrict the values assignable to the port number to those between 49152 and 65535 (inclusive).

I wrote some test methods that test that anything outside of this range causes the test to fail. They do fail (as expected, as the code does not yet take that into account).

So my question is: what is the best place to put the code that forces an invalid value to a valid value - here:

public int Port
{
    get
    {
        return port;
    }
    set
    {
        port = value;
    }
}

such as:

public int Port
{
  get
  {
      return port;
   }
   set
   {
       if ((value < 49152) || (value > 65535))
       {
          value = 55555;
       }
       port = value;
    }
}

...or somewhere else?

Who is DJKraze? Actually, I think we do need it, as numbers below 49152 are possibly already in use, and numbers above 65535 are not available. The people entering the port numbers may not know that. — B. Clay Shannon-B. Crow Raven, Feb 06 '12 at 19:24
This is a duplicate of a question I had to post to settle an argument: http://stackoverflow.com/questions/635519/business-validation-logic-code-smell You absolutely should not be assigning over the caller. It hides bugs. It's dangerous (what if the caller meant to type 49152 but accidentally typed 49151, now you've glossed over this terrible error with 55555 letting the program continue with a value completely different than what the caller intended). — jason, Feb 06 '12 at 20:05
Or what if you accidentally introduce a bug in assigning to `Port`? Say you're drunk coding, and you write `x.Port = Int32.Parse(s.Substring(s.Length - 1));` instead of `x.Port = Int32.Parse(s);` and so now all your values come in as garbage. You've let this disastrous bug come into your program because of this. Don't do this. — jason, Feb 06 '12 at 20:10
I added the validation tag, since this is really what the question is about. — qxn, Feb 06 '12 at 20:27

score 15 · Answer 1 · answered Feb 06 '12 at 19:27

15

what is the best place to put the code that forces an invalid value to a valid value?

The best place for that is nowhere. Forcing an invalid value to a random valid value is hiding a bug in the caller. The caller can know ahead of time whether the value they are attempting to set is valid or not, so crash them if they do it wrong. Throw an exception, don't ignore the bug and make a guess as to what they meant.

answered Feb 06 '12 at 19:27

Eric Lippert

647,829
179
1,238
2,067

I had to make this exact point to a client the other day. They have a field that is essentially a weighting multiplier in a calculation, but it is an optional value. They specified that 0 be treated as 1 in their spec. I had to persuade them that a value of 0 should be treated as a throw an exception as it is not a valid value (0.01 to 2.5 were the valid range) and so indicates a bug that needs to be fixed whereas null is valid but should be treated as 1 so as to have no affect on the weighting calculation. – Ben Robinson Feb 06 '12 at 20:16

score 12 · Accepted Answer · edited Feb 06 '12 at 20:15

12

You can put validation logic in your setter, but you should probably let somebody know if the validation fails.

ie:

private static int minimumPortNumber = 49152;
private static int maximumPortNumber = 65535;

public int Port
{
  get
  {
      return port;
   }
   set
   {
       if ((value < minimumPortNumber) || (value > maximumPortNumber))
       {
           throw new ArgumentOutOfRangeException("port", string.Format("The port number is out of range. It must be between {0} and {1}", minimumPortNumber, maximumPortNumber));
       }
       port = value;
    }
}

edited Feb 06 '12 at 20:15

SolutionYogi

31,807
12
70
78

answered Feb 06 '12 at 19:19

qxn

17,162
3
49
72

2

I like this answer, but consider throwing an exception from a setter to be indicative of a bug (i.e., it's OK if a setter throws exceptions on invalid input, but the caller of the setter should validate the input to prevent such exceptions from being thrown). As a further note, IMO port should have `static readonly` minport/maxport member. `49152` and `65535` strike me as being magic numbers. – Brian Feb 06 '12 at 20:01
I completely agree with Brian. Error message should let the developer know what the expected values are, don't just tell them they are wrong, tell them why they are wrong. – SolutionYogi Feb 06 '12 at 20:16

score 2 · Answer 3 · answered Feb 06 '12 at 19:19

2

This is one of the biggest benefits to having wrapper methods around the fields and not exposing them directly. So I would say most definitely, yes.

answered Feb 06 '12 at 19:19

Brandon Moretz

7,512
3
33
43

Is the property accessor/mutator (getter/setter) the right place to enforce acceptable ranges?

3 Answers3

Linked