This post is the third in a series about Poka-yoke Design - also known as encapsulation.

Automatic properties are one of the most redundant features of C#. I know that some people really love them, but they address a problem you shouldn't have in the first place.

I totally agree that code like this looks redundant:

private string name;
public string Name
{
    get { return this.name; }
    set { this.name = value; }
}

However, the solution is not to write this instead:

public string Name { get; set; }

The problem with the first code snippet isn't that it contains too much ceremony. The problem is that it breaks encapsulation. In fact

“[…] getters and setters do not achieve encapsulation or information hiding: they are a language-legitimized way to violate them.”

James O. Coplien & Gertrud Bjørnvig. Lean Architecture. Wiley. 2010. p. 134.

While I personally think that properties do have their uses, I very rarely find use for automatic properties. They are never appropriate for reference types, and only rarely for value types.

Code Smell: Automatic Reference Type Property

First of all, let's consider the very large set of properties that expose a reference type.

In the case of reference types, null is a possible value. However, when we think about Poka-yoke design, null is never an appropriate value because it leads to NullReferenceExceptions. The Null Object pattern provides a better alternative to deal with situations where a value might be undefined.

In other words, an automatic property like the Name property above is never appropriate. The setter must have some kind of Guard Clause to protect it against null (and possibly other invalid values). Here's the most fundamental example:

private string name;
public string Name
{
    get { return this.name; }
    set 
    {
        if (value == null)
        {
            throw new ArgumentNullException("value");
        }
        this.name = value; 
    }
}

As an alternative, a Guard Clause could also check for null and provide a default Null Object in the cases where the assigned value is null:

private string name;
public string Name
{
    get { return this.name; }
    set 
    {
        if (value == null)
        {
            this.name = "";
            return;
        }
        this.name = value; 
    }
}

However, this implementation contains a POLA violation because the getter sometimes returns a different value than what was assigned. It's possible to fix this problem by adding an associated boolean field indicating whether the name was assigned null so that null can be returned from the setter in this special case, but that leads to another code smell.

Code Smell: Automatic Value Type Property

If the type of the property is a value type, the case is less clear-cut because value types can't be null. This means that a Null Guard is never appropriate. However, directly consuming a value type may still be inappropriate. In fact, it's only appropriate if the class can meaningfully accept and handle any value of that type.

If, for example, the class can really only handle a certain subset of all possible values, a Guard Clause must be introduced. Consider this example:

public int RetryCount { get; set; }

This property might be used to set the appropriate number or retries for a given operation. The problem with using an automatic property is that it's possible to assign a negative value to it, and that wouldn't make any sense. One possible remedy is to add a Guard Clause:

private int retryCount;
public int RetryCount
{
    get { return this.retryCount; }
    set
    {
        if (value < 0)
        {
            throw new ArgumentOutOfRangeException();
        }
        this.retryCount = value;
    }
}

However, in many cases, exposing a primitive property is more likely to be a case of Primitive Obsession.

Improved Design: Guard Clause

As I described above, the most immediate fix for automatic properties is to properly implement the property with a Guard Clause. This ensures that the class' invariants are properly encapsulated.

Improved Design: Value Object Property

When the automatic property is a value type, a Guard Clause may still be in order. However, when the property is really a symptom of Primitive Obsession, a better alternative is to introduce a proper Value Object.

Consider, as an example, this property:

public int Temperature { get; set; }

This is bad design for a number of reasons. It doesn't communicate the unit of measure and allows unbounded values to be assigned. What happens if - 100 is assigned? If the unit of measure is Celcius it should succeed, although in the case when it's Kelvin, it should fail. No matter the unit of measure, attempting to assign int.MinValue should fail.

A more robust design can be had if we introduce a new Temperature type and change the property to have that type. Apart from protection of invariants it would also encapsulate conversion between different temperature scales.

However, if that Value Object is implemented as a reference type the situation is equivalent to the situation described above, and a Null Guard is necessary. Only in the case where the Value Object is implemented as a value type is an anonymous property appropriate.

The bottom line is that automatic properties are rarely appropriate. In fact, they are only appropriate when the type of the property is a value type and all conceivable values are allowed. Since there are a few cases where automatic properties are appropriate their use can't be entirely dismissed, but it should be treated as warranting further investigation. It's a code smell, not an anti-pattern.

On a different note properties also violate the Law of Demeter, but that's the topic of a future blog post…


Comments

James Nail
Hi Mark,
I'm enjoying this series, and you can be sure I'll be sharing it with my teammates.
I wanted to point out a typo that might confuse readers, though -- you used the term "anonymous properties" instead of "automatic properties" toward the end of the article, which threw me off at first.
Anyway, keep up the great work!
2011-05-26 14:07 UTC
Omer Katz
What about NetworkStream's Connected property?
It has a private set and a public get and returns a boolean that indicates if the underlaying socket is connected.
Nothing wrong with that.

By your logic, any .NET ORM is bad because it uses properties and violates the Law of Demeter.
2011-05-26 14:09 UTC
Isn't the code smell "public setters" rather than automatic properties? Making the set protected or private solves this issue. E.g.

public decimal MyValue { get; private set; }

With such usages it is legitimate for the guard code to be in a state mutating method, constructor validation etc
2011-05-26 14:15 UTC
Good points, and I completely agree with the { get; set; } kind of auto-implemented properties - in fact I don't think I've ever checked one in.

However, I'm forever using the {get; private set; } kind of auto-implemented properties - what are your opinions on those? What about a hypothetical and confusingly named { get; readonly set; } auto-implemented property?
2011-05-26 14:25 UTC
James, thanks for pointing that out. I've now corrected the error.
2011-05-26 18:31 UTC
Omer, a boolean property falls into that rare case described during the discussion of value type properties. If the class exposing the property truly can accept any value that the type can possibly take, an automatic property is in order. A boolean value can only take one of two values, and both are legal. Thus, no Guard Clause is required. However, please notice that this scenario is not (or at least should not be) representative.

Regarding ORMs you are now jumping ahead of me :) I'll come to that in a later blog post, but yes, I genuinely believe that the whole concept of an ORM (at least in any incarnations I've seen so far) is a fallacy.
2011-05-26 18:39 UTC
Ben, you are correct that the issue is at least most prevalent when the setter is public. However, keep in mind that we should always treat protected as public, as anyone can always come by and derive from a class.

For private setters it's not so clear-cut. In most cases when I have read-only properties it tends to be because the property is being supplied through the constructor and subsequently treated as immutable. When that is the case, I much prefer a backing field because it can be declared as readonly.

In other words I rarely write code where the class itself can mutate the value of a read-only property, but I know that other people do. When this is the case, I could argue that I'd still prefer the setter to protect the invariants of the class, but that argument tends to become a little silly because with a backing field the class could always just mutate the value directly. In any case, if one has a class that needs that kind of protection from its own internals one probably has more serious problems :)

Keep in mind that this whole discussion about encapsulation relates to the public API of classes. Obviously, once you start poking around in the internals of a class, you're already looking at the source code and thus past the encapsulation perimeter.
2011-05-26 18:50 UTC
Alex, see my previous comment regarding { get; private set; }

Regarding { get; readonly set; } I would probably use something like this assuming this was something that could only be used from the constructor, because I'd still be able to add the appropriate Guard Clause in the constructor before invoking the setter.
2011-05-26 18:54 UTC
I totally agree with your post. However, there are some cases when you do not need to make checks for validity for reference types. If a class with such a property does not provide a default non-null value for it, then any other checks are redundant as the user may set or may not set a value to that property. This way it will still have its default value of NULL.

Using properties with public setters in really tricky. When you have a class with properties and methods, users do not know which properties should be set before a method is called which will result in a run-time exception. That's why required parameters should be provided directly to constructors and methods.
2011-05-27 07:52 UTC
If the value is optional a public setter is in order. However, allowing the field to be null is not, as it would require the rest of the class to have to check for null every time it uses the field. A Null Object is a better alternative, which again leads to the rule that a Null Guard is always appropriate for reference types. However, in certain cases, that Null Guard might not throw an exception, but rather replace the null with a Null Object.
2011-05-29 08:55 UTC
Theo Andersen
Hi Mark,
I really like your design smell series posts, and i have a few questions.

I get and agree on your points about encapsulation and Automatic Properties, but as i see it a lot of frameworks forces you to break this encapsulation. An example is XML Serialization (ISerializable), which needs an empty constructor and then access to all the public properties to set them.

Couldn't this lead to both the automatic property and temporal coupling smells, as you need to have basic properties so the framework can set them, plus an empty constructor which then breaks your temporal encapsulation?
To adhere to the serialization interface you're forced to open up your code (or create a DTO).

2011-05-30 09:18 UTC
Theo, please see my latest post that touches on exactly that point.
2011-05-31 18:11 UTC
I wonder if there is a more elegant way to do this than creating guard clauses for every reference type property. It would have been nice to do this in a more declerative way. I posted a question on SO as well: http://stackoverflow.com/questions/6773189/auto-implemented-properties-with-non-null-check
2011-07-21 16:38 UTC
Ben Robinson
Your points are arguments against using automatic properties inappropriately and are far from general cases. There are perfectly valid reasons to allow the setting of properties to null (e.g. using ORM how would you set a property mapped DB field to null) i think your example of a setter that throws a null reference exception is a code smell itself unless you have a specific reason for disallowing it. Your example of the RetryCount is simply an inappropriate use of an automatic property not at all an argument against automatic properties. Your example of temperature is an example of inappropriate type and has nothing to do with automatic properties at all, with the correct type an automatic property would make perfect sense as you could build the validation into the Temperature class. Automatic properties as just shorthand for simple field backed properties and where simple field back properties are correct then so are automatic properties. Simple field backed properties with no validation are sometimes used when they shouldn't be, but that has nothing to do with automatic properties.
2011-09-02 08:38 UTC
I agree with Ben's last point, I believe automatic property(or simple field backed property) is just a way to deal with how C# compiler does assembly linking, and using simple properties allows minor modifications to them without recompiling all dependent assemblies.

I somewhat agree to your Guard Clause argument, it's something I should pay more attention to instead of blindly implementing automatic properties. But I do have some doubts, in many cases, there is no proper default value, like Birth Date, Country, etc. In fact, I think that's the case most of the time, how would you deal with those situations?
2012-01-10 17:04 UTC
2012-01-15 16:40 UTC


Wish to comment?

You can add a comment to this post by sending me a pull request. Alternatively, you can discuss this post on Twitter or Google Plus, or somewhere else with a permalink. Ping me with the link, and I may add it as a comment.

Published

Thursday, 26 May 2011 13:33:13 UTC

Tags



"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!