6

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

6

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.

3

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 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 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 at 14:05
  • 1
    @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 at 14:50
1

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;
        }
    }
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("-", "--").

  • 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
0

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;
    }
}
0

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; }

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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