Is it appropriate to process a property in the setter?

Question

I have a class with a Name property, where the name is set by the end-user. In order to ensure compatibility with the rest of the environment, and to avoid rejecting a lot of inputs, some processing of the name is done automatically:

public string Name {
    get { return _Name; }
    set {
        if (value == null)
            return;
        _Name = value.Replace("/", "-").Replace("%", "pct").Replace("&", "-").Replace("'", "").Trim();
        _RawName = value;
    }
}

My concern is about encapsulation and SRP.

I wonder if something like no processing, and then a method such as myObject.processName() would be better from a principles point of view (and if I am right thinking it is not as clear as my current implementation).

I have carefully read a similar question, but is discusses validation only, and this goes further, and the data is manipulated.

-- There is no beginner tag here, so let me put it here. I am new to all this, please forgive any stupid questions :)

Possible duplicate of [Is it considered a bad practice to add logic in a property setter?](https://softwareengineering.stackexchange.com/questions/204739/is-it-considered-a-bad-practice-to-add-logic-in-a-property-setter) — gnat, Oct 27 '17 at 13:50
`Name = Name;` having a significant effect is dubious. Why don't you add a public `RawName` property and assign to it? — CodesInChaos, Oct 27 '17 at 14:08
Hang on, if you set it to null you ignore that and keep the old field values? — Nathan Cooper, Oct 27 '17 at 15:20

score 7 · Answer 1 · edited Mar 20 '19 at 07:48

This breaks the principle of least surprise. For a property with a getter and setter, you would assume them to be symmetric, i.e. you get the same thing as you set, at least semantically. Here you set one thing but you get a different thing which is some kind of sanitized or encoded string where some information have been removed.

You probably shouldn't even represent these two kind of values (raw and sanitized names) as bare strings, since you are bound to mix them up at some point.

You could have two properties - RawName and SanitizedName. SanitizedName should not have a setter though, since the encoding is one-way.

TheCatWhisperer · Answer 2 · 2017-10-27T16:03:20.060

No, this is not fine; however, it is not the worst thing in the world either. I would reject a PR with this in it, but I would not get mad at you ;)

Let's say you deploy this to production... a few months go by and then you get a email from your PM:

Max, the user wants to know why the "%" was removed from his name, they are very annoyed as the "%" is indicative of their title in their country. Now, I know there is a reason you have to do that, so if the user enters a weird character, just display to them a message explaining that is will be stripped.

A better place to do the sanitation would be the getter, that makes your class more flexible, you can then add another property bool WasSanitized, then you don't need the extra RawName field.

Better still, don't have this logic in this class at all. It is a common mistake to think that user input sanitation is a business concern; but, it is not. Basic input validation does not belong in a business object. If you have all these considerations in this class, the class has too many cross-cutting responsibilities... it has gone from modeling a business entity to modeling a business entity and validating user input.

Basic user input cleansing is a UI concern, keep this code in your UI layer, your code base will be much cleaner. Keeping this logic in the class puts you at risk for the "ambiguous viewpoint" anti-pattern.

Interesting you'd say classes should not be concerned with validation. Data Annotation on properties in C# used for validation, is out of the box behaviour no? — niico, Apr 28 '19 at 16:13
@niico I said no such thing, I only said that it should not be in *that* class. It should be in a separate class that specialized in that concern. — TheCatWhisperer, Apr 29 '19 at 13:39
So in essence are you saying validation should be in ViewModels, assuming the OP's model is a Model not a ViewModel? — niico, Apr 30 '19 at 14:05
@niico essentially. I am not saying *all* validation should be in the view model, but basic for input validation like this, it makes sense. Also, if this validation is needed multiple times, it could also go in an intermediate class. — TheCatWhisperer, Apr 30 '19 at 14:50

score 2 · Answer 3 · answered Mar 21 '19 at 06:39

In this case Name is not a string...

Your business logic is about names, so why not actually represent what a Name is, not how it is stored?

class Name
{
   static bool Validate(string name);
   static string CanonicalFormat(string name);

   Name(string name)
   {
       if (!Validate(name))
           throw Exception(...);

       Raw = name; //the captured version
   }

   string Raw { get; }
   string Canonical { get { return CanonicalFormat(Raw); } } //cache it if you wish...
   string toString() { return ...Raw or Canonical....; }

   ... //whatever else name can do...
}

And then the Business logic can be about names:

Name Name { get; set; }

score 1 · Answer 4 · answered Oct 27 '17 at 16:58

I think this is clearer as it does not mix concerns with a single property.

 public class Name
    {
        public string Raw { get; set; }

        public string Formatted
        {
            get
            {
                return string.IsNullOrWhiteSpace(Raw) ? null : Raw.Replace("/", "-").Replace("%", "pct").Replace("&", "-").Replace("'", "").Trim();
            }
        }

        public Name(string raw)
        {
            Raw = raw;
        }
    }

score 0 · Answer 5 · answered Oct 27 '17 at 13:59

0

I think it is fine.

What you would not want is if it is run again on the output then it produces different output. For example Replace("-", "--").

answered Oct 27 '17 at 13:59

paparazzo

1,937
1
14
23

4

why do you think this is fine? what is your justification? – TheCatWhisperer Oct 27 '17 at 16:19
@TheCatWhisperer Formatting in the setter seems to be an accepted practice. I don't see how it could break anything in this example. – paparazzo Oct 27 '17 at 16:21

John Wu · Answer 6 · 2019-03-21T02:56:24.943

The best solution for an end user would be to prevent them from entering the incorrect characters to begin with, e.g. by filtering the keystrokes in the textbox or checking the value when the OK button is pressed (if WinForms or WPF), or applying a RegEx validator or custom Javascript (if a web page).

The property should then validate the name, but not mangle it.

Yes, this is redundant validation, which is okay, and often unavoidable, especially in a multi-tier application.

public string Name
{
    get
    {
        return _Name;
    }
    set 
    {
        if (value == null) throw ArgumentException();
        var cleansed = value.Replace("/", "-").Replace("%", "pct").Replace("&", "-").Replace("'", "").Trim();
        if (cleansed != value) throw ArgumentException();
        _Name = value;
    }
}

Is it appropriate to process a property in the setter?

6 Answers6