Writing C# utility that checks on property

Question

I have a business requirement that requires checking on a person's first name to ensure it does not have the character "1". The model:

public class Person
{
        public string FirstName { get; set; }
}

I started writing the following function to perform the check:

public Person CleanPersonFirstName(Person person)
{
    var clean = person;

    if (clean.FirstName.Contains("1"))
    {
        clean.FirstName.Replace("1", "");
    }

    return clean;
}

But I am not sure if it is justifiable creating a new copy of the Person object, perform the check, and return the copy.

Would the following be a better approach?

public void CleanPersonFirstName(ref Person person)
{
    if (person.FirstName.Contains("1"))
    {
        person.FirstName.Replace("1", "");
    }
}

[Edit] - Thank you all for all comments posted. I would consider this thread to have gathered sufficient feedback to get me thinking.

[Edit2] - Stumbled across this article on pure function (context is JS, but the message is still applicable): https://medium.com/javascript-scene/master-the-javascript-interview-what-is-a-pure-function-d1c076bec976

Key messages:

// impure addToCart mutates existing cart
const addToCart = (cart, item, quantity) => {
  cart.items.push({ // Here the input gets mutated.
    item,
    quantity
  });
  return cart;
};

As mentioned by the article: The problem with this is that we’ve just mutated some shared state.

Revised version:

// Pure addToCart() returns a new cart
// It does not mutate the original.
const addToCart = (cart, item, quantity) => {
  const newCart = lodash.cloneDeep(cart);

  newCart.items.push({
    item,
    quantity
  });
  return newCart;

};

Answer 1

This is not the way to implement such a requirement. Rather than let the property be changed and then later check if its value is OK, make sure it never gets set to a bad value in the first place. Start by declaring a proper private member firstName and modify your property like so:

public class Person
{
    public string FirstName
    {
        get { return this.firstName; }

        set 
        {
            this.ValidateFirstName();
            this.firstName = value;
        }
}

If the first name is no good, have ValidateFirstName throw an exception.

Alternatively, make it a value type, have a constructor with arguments and do your validation in the constructor.

Answer 2

The first thing I stumbled across in your question was, first it asks for a "check" of a business rule, but then it presents some code which does more than just a check, it starts to correct the person's name in some way. So it actually does two things in one (checking and cleaning), which violates the single responsibility principle,

So first, I would recommend to separate validation from correction, these are different concerns. Instead, your Person class could have a method bool IsFirstNameValid(). The cleaning then should be done somewhere else.

Second, there are some other answers suggesting to make sure a Person object will never be in an invalid state. That is not wrong, but to my experience it is quite unrealistic for many real-world cases. Often, the Person data comes from an external data source like a database, a file, a web service or a web form / GUI, so one needs to build the object first and do the validation of certain attributes afterwards. If your system works like this, expect the Person object to be in an invalid state, at least temporarily.

So if you know where the Person objects comes from and have an idea about how it will be used in the context of your system, that gives you a foundation on if and how to implement the actual cleaning. The "cleaning" process can look differently depending on the context, like

• rejecting the object from the GUI / form where a user has entered it

• throwing an exception and let the caller handle the problem

• silently replace the "1" by "", as your example shows it (which has a certain risk of masking an error)

• replace the "1" by "", but log this action somewhere

If your Person object will be reused in different contexts, it may be required to implement more than one of those alternatives, but all can reuse the same IsFirstNameValid method.

In case you indeed need a CleanFirstName method, you will still have to decide if you prefer a mutable or an immutable approach, but for both cases, I would suggest you make the cleaning a method of the Person class itself. The mutable version may simply look like this

public class Person
{
    public Person CleanFirstName()
    {
        FirstName.Replace("1", "");
    }
}

and the caller - in its context - can decide if it is better to use it either like

  if(!person.IsFirstNameValid())
  {
      person.CleanFirstName();
  }

or

  if(!person.IsFirstNameValid())
  {
      log.InvalidFirstName(person.FirstName);
      person.CleanFirstName();
  }

or

  if(!person.IsFirstNameValid())
  {
      var cleaned = new Person(person);   // assuming there is a copy constructor
      cleaned.CleanFirstName();
      return cleaned;
  }

or

  // no automatic cleaning at all
  if(!person.IsFirstNameValid())
      throw SomeException("...");

So, in short, for making a founded decision about how the cleaning process should look in detail, you need more context. If, for now, you do not have enough context, you can still provide the building blocks for a sensible validation, and let the callers decide how they use it.

Answer 3

As mentioned by Erik Eidt, the code above mutates the existing object in both cases. If Person was a struct instead of a class, then it would work the way you're implying.

A non-mutating version could look something like:

public Person CleanPersonFirstName(Person person)
{
    if(person.FirstName.Contains("1"))
    {
        return new Person()
        {
            FirstName = person.FirstName.Replace("1", string.Empty);
        };
    }

    return person;        
}

In this case, I don't think either is the correct approach. If it's a rule that a Person can't have an invalid FirstName, then it shouldn't be possible to have a Person in that state to begin with. Make FirstName immutable and construct Person instances via a factory / builder that applies the necessary validation.

In general, mutable vs immutable is a highly debated topic. Personally, I would go with an immutable design unless there's a good reason to make it mutable - usually performance, but sometimes it's a more natural representation.

If you're worried about performance though - measure it. Making changes based on what you guess is faster is usually a bad idea.

Answer 4

The obvious and most common solution would be something like this:

person.FirstName = Clean(person.FirstName);

This certainly makes it unambiguous what is mutating, and it is quite straightforward to write Clean(string) as a pure function.

A more encapsulated solution might be:

person.CleanFirstName();

And if there is no globally correct rule, and you need to be able to vary it, you can always make CleanFirstName a partial function, e.g. pass in a delegate:

class Person
{
    public void CleanFirstName(Func<string,string> filter)
    {
        this.FirstName = filter(this.FirstName);
    }
}

//Usage:
var filter = s => s.Replace("1", string.Empty);
person.CleanFirstName(filter);