1

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;

};
  • 2
    var clean = person; is merely a reference assignment, no clone or copy has been made: both clean and person refer to the same exact instance. So, between your two options, they are not different: both modify the one and only Person object involved. FYI, when you get that far, I favor returning a new object rather than modifying the one in place. – Erik Eidt Feb 21 '19 at 0:10
  • Touche Erik. What's your rationale behind returning a new object rather than modifying the one in place? – taylorswiftfan Feb 21 '19 at 0:39
  • 1
    About the rationale - search for something like "immutability pros and cons". Immutability just refers to classes/objects that are designed so that you cannot change (mutate) their internal state once it's set; you can only create a new object. E.g., in C#, the string class is immutable; all operations return a new string instance. But, it would be great if you edited the question to tell us where and how you are using this code (in UI/Views, or in the business (domain) layer), what calls the clean method (is it a UI validator of some sort), etc., as it may affect the answer to your question. – Filip Milovanović Feb 21 '19 at 6:08
  • Also, it may be relevant (for giving more context about how the code is used and where in the application it appears) if it's a desktop app (e.g., WPF, and whether the Person used in data binding or not), or a web application (when you say model, is it something like an MVC view model, or domain model (server side), etc.). – Filip Milovanović Feb 21 '19 at 6:14
  • 4
    Your code does not work. Non of your approaches. Before you worry about structure, make sure your code does what you want it to do. This is commonly called testing. It's somewhere between impossible and too lengthy to suggest improvements to code that does not do what you think it does in the first place. – nvoigt Feb 21 '19 at 7:50
3

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.

  • Note that some developers argue that this is counterintuitive behavior, because external callers expect that when they set a value, that this exact value is retrievable via the same property. e.g. myFoo.Bar = 123; Assert.AreEqual(myFoo.Bar, 123);. I didn't used to agree with that but I've come around to it after seeing this abused too many times. A more intuitive solution would be to leave FirstName as an autoproperty (no cleaning), and add a readonly property CleanFirstName => CleanFirstName(this.FirstName). It offers the same functionality but it's (imo) more intuitive. – Flater Mar 26 '19 at 9:44
  • 1
    @Flater In this answer, the client will get exactly what they set if the value is valid. If the value is not valid, an exception is thrown and the property will not be updated. – Hieu Le Mar 26 '19 at 10:40
  • @HieuLe: Fair enough; though OP is looking to silently clean the name. Martin did mention throwing an exception but the throwing of the exception becomes more of a necessity (for good practice) than the answer seems to suggest. – Flater Mar 26 '19 at 11:02
  • Considering both the answer and comments, I would argue to make Person { get; private set; } and then have a SetFirstName() method which handles any validation logic (whether this being a silent clean or throwing an exception). – Noceo Mar 26 '19 at 15:12
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.

0

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.

  • Thanks Errorsatz. Appreciate you being able to understand the message behind my question. So in this context, a mutable design is where the same instance is cleaned and returned, while an immutable design is where a new instance is created with cleaned data? – taylorswiftfan Feb 21 '19 at 3:04
  • 1
    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. This idea (whose intent I do agree with) tends to go out the door when you deal with serialization, which requires public setters and getters for properties. In these cases, rather than have an vague serialization exception, you're better off with an explicit and targeted validation (and subsequent error message) after the serialized data is instantiated, which means you can't just block object creation. [..] – Flater Feb 21 '19 at 8:27
  • 1
    [..] There may also be cases where the validation is not a globally correct rule and only belongs to a subset of code. E.g. people can have a 1 in their name, but when taking a particular action (e.g. generate an email address for them) will cause a person's name to be cleaned. – Flater Feb 21 '19 at 8:29
0

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

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.