Don't make setters of properties in your class public!

#dotnet #csharp #cleancode

Introduction

In this post I'm explaining why public setters of properties can introduce bugs to our code and how to ensure better control over the data integrity in C# class.

OK, so what is wrong with public setters?

In my entire live I have seen a lot of classes similar to this one:

public class Car
{
    public string Color { get; set; }
    public string Brand { get; set; }
    public List<Wheel> Wheels { get; set; }

    public Car(string color, string brand)
    {
        Color = color;
        Brand = brand;
        Wheels = CreateFourWheels();
    }
}

At first glance you may think that everything is fine with such class. I'll try to explain what in my opinion is broken - that thing is public setters. With such constructed class, nothing stops a client of that class from doing such operation:

var car = new Car("White", "BMW");
// some logic here
car.Brand = "Toyota";

What happened here? Someone did operation which was not allowed. A client of our class performed a change which makes no sens regarding to the business domain. A brand of the car cannot be changed during the whole life of the car. It was possible because of the public setter.
Conclusion numer 1: in most cases with public setter we lose the control of the data integrity of our model.
We can fix that by changing setter's accessibility to private. After such change a client of our class is not able to change the value of the property, it will end with a compilation error.

OK, we have fixed the setter accessibility. Is everything fine right now and are we done? Not really. I'd like to emphasise two things more which are critical from the data integrity point of view. One of these two things can be quite tricky and not obvious.

Let's assume that if a Ford car is being constructed it can have only black color. Any other color is not allowed. Let's change our constructor to this:

public Car(string color, string brand)
{
  if (brand == "Ford" && color != "Black")
  {
    throw new Exception("Ford is allowed only in black color");
  }

  Color = color;
  Brand = brand;
  Wheels = CreateFourWheels();
}

We added a simple check and if the condition is not met, an exception is thrown. It is not the ideal solution but I did it to point out that every change of the data should be validated. If we had the method which makes repainting possible, we'd add a similar check to the method as well.

public void Repaint(string color)
{
  if (Brand == "Ford" && color != "Black")
  {
    throw new Exception("Ford is allowed only in black color");
  }

  Color = color;
}

The example of validation is simplified in the code above, I'm planning to create another post and cover this topic so I don't want to focus on this topic too much in this article.

Let's take a look at our class once again:

public class Car
{
   public string Color { get; private set; }
   public string Brand { get; private set; }
   public List<Wheel> Wheels { get; private set; }

   public Car(string color, string brand)
   {
       // the content of constructor
   }
}

Let's focus on Wheels property. It doesn't have a public setter. Is everything set properly then? Not really, there is one small gotcha. It is still possible to change the content of the collection, a client of our class can perform operations like these:

var car = new Car("White", "BMW");
car.Wheels.Add(new Wheel()); // (a)
car.Wheels.Clear();          // (b)

In the line (a) we added new additional wheel despite the fact that our wheel collection does not have public setter but a private one. If our logic assumes that every car has 4 wheels, operation of adding 5th wheel should not be allowed. In the line (b) we clear the collection and it becomes an empty collection. That should be forbidden too.

Why it is possible you may ask? It is possible because changing the setter of collection to a private one blocks only assigning a new value to the collection. The property is accessible and client code can invoke methods on it.

car.Wheels = new List<Wheel>(); // compilation error
car.Wheels.Clear();             // still possible

How we can improve that? We have two options:

public class Car
{
  private List<Wheel> _wheels;
  public string Color { get; private set; }
  public string Brand { get; private set; }
  public List<Wheel> Wheels { get { return _wheels.ToList(); } }

  public Car(string color, string brand)
  {
     _wheels = CreateFourWheels();
  }
}

In first option we changed the logic behind Wheels property. We use private field _wheels internally and client of the class can reach only a copy of the collection (by using .ToList() method). Even if the copy is changed, the original field remains the same.

public class Car
{
  private List<Wheel> _wheels;
  public string Color { get; private set; }
  public string Brand { get; private set; }
  public ReadOnlyCollection<Wheel> Wheels
  { 
    get { return new ReadOnlyCollection<Wheel>(_wheels); } 
  }

  public Car(string color, string brand)
  {
     _wheels = CreateFourWheels();
  }
}

The second way of providing the data integrity is using ReadOnlyCollection type. After such change client code is not able to call any method which changes the content of the collection.

A summary:

In most cases with public setter we lose the control of the data integrity in our model. We can resolve that by using private setters,
Every change of the data should be validated in constructor or in the method / a setter,
Collections need special treatment. We should expose a copy of the collection calling .ToList() method or use ReadOnlyCollection.

Thank you for reading to the end! I hope that my tips will help you to create more reliable code.

If you like my posts, please follow me on Twitter :)