Is it ok to place logic in setter for C# property?

Question

I'm looking at some old code and I'm curious if it's appropriate to have logic within the setter property, or if there's a much better way to write this and why? Thank you!

        string qBankCode;
        string qCode;
        public string QBankCode
        {
            get => qBankCode;
            set
            {
                if (value.Length == 0)
                    throw new ArgumentException("You need to enter the question bank code as it is mandatory.");
                if (value.Length > 30)
                    throw new ArgumentException("Question bank code cannot exceed 30 characters.");
                qBankCode = value;
            }
        }
        public string QCode
        {
            get => qCode;
            set
            {
                if (value.Length == 0)
                    throw new ArgumentException("You need to enter the question code as it is mandatory.");
                if (value.Length > 30)
                    throw new ArgumentException("Question code cannot exceed 30 characters.");
                qCode = value;
            }
        }

Noting in the language is stopping you.. So what your asking is our opinion on how much logic or what sorts of logic/validation should be put there, which is opinionated an off-topic, also there is a wealth of information on the internet about this sort of situation and what is considered ok — TheGeneral, Sep 25 '19 at 23:38
https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.stringlengthattribute?view=netframework-4.8 — Arphile, Sep 25 '19 at 23:40
In order to write a unit test around it I'd need to pull the logic out into it's own method to the logic by itself right? — Blobula, Sep 25 '19 at 23:43
_In order to write a unit test around it I'd need to pull the logic out into it's own method_ I don't see why: set the value of the property to an invalid value, and assert it throws, But I have to say I wouldn't be happy to see this code in our code base: the value should be validated **before** it reaches the setter, and that's what I would use a method for. And then you can get rid of the backing fields. — stuartd, Sep 25 '19 at 23:58
@Arphile unless there's something in the stack to actually check the attribute, it will have no effect - they're not 'magical'. — stuartd, Sep 26 '19 at 00:04
https://stackoverflow.com/questions/2923116/is-it-a-good-practice-to-implement-logic-in-properties — TheGeneral, Sep 26 '19 at 00:13
`if there's a much better way to write this` Given your property setters are very similar, considering putting that validation code into a function and sharing it between the two setters. — mjwills, Sep 26 '19 at 00:18
https://stackoverflow.com/questions/6127290/c-sharp-add-validation-on-a-setter-method — Rufus L, Sep 26 '19 at 00:25
If you are going to do that, consider having functions like `bool TrySetQCode(string val)`. It does the same validation. If it succeeds, it sets the prop value and returns true, if it fails, it returns false without changing the property value. This allows your users not to have to replicate your validation logic and reduces the likelihood of an exception. Also, consider using `string.IsNullOrEmpty` instead of testing the string length (it prevents someone from assigning null to the property) — Flydog57, Sep 26 '19 at 00:36

score 4 · Answer 1 · edited Jun 20 '20 at 09:12

According to the Framework Design Guidelines:

It is acceptable for setter's to throw exceptions

✓ DO preserve the previous value if a property setter throws an exception.

✓ DO allow properties to be set in any order even if this results in a temporary invalid state of the object.

as long as they are not reliant on the state of other properties eg

public string QCode
       { ... set {...
                if (QBankCode.Length ==10 && value.Length % 2 == 0)
                    throw new exception();...

You code sample meets this but you should make sure your properties have sensible defaults

✓ DO provide sensible default values for all properties, ...

score 3 · Answer 2 · answered Sep 26 '19 at 01:01

Is it OK?

I don't think it's generally OK to have logic in property setters, but your example shows validation which is not only OK, but good practice.

Is there a better way?

or if there's a much better way to write this and why?

It really depends on how the code is used.

For example, for MVC or EntityFramework models one can argue for StringLength and Required attributes. Please see Adding Validation to the Model (C#) or StringLengthAttribute Class .

If it's really important it should be validated on multiple levels (throw and use attributes).

Attributes

Please note that attributes require a framework (MVC,EF, etc.) to enforce them, while throw new ArgumentException will work in all circumstances which may be the desired option.

Contract

Please note that the exceptions are not visible in the build versions of the library and if your models are distributed as a binary it is important to communicate the validation rules via some other way.

Internationalisation and end user strings in general

Please note that while the validation exceptions are a sure what to stop bad values if you have to support multiple languages or allow changing the message without deploying new code you have to come up with some other way of dealing with validation failures.

How much validation is OK?

Microsoft's Property Design .NET Guide states

✓ DO allow properties to be set in any order even if this results in a temporary invalid state of the object.

It is common for two or more properties to be interrelated to a point where some values of one property might be invalid given the values of other properties on the same object. In such cases, exceptions resulting from the invalid state should be postponed until the interrelated properties are actually used together by the object.

I strongly disagree with such design, but as always there are scenarios where it makes sense.

I have found that systems that keep objects in a valid state are easier to work with and I always encourage immutable class design first.