It is not an inherently bad design, but like any powerful feature it can be abused.
The point of properties (implicit getter and setter methods) is that they provide a powerful syntactic abstraction allowing consuming code to treat them logically as fields while retaining control for the object that defines them.
While this is often thought of in terms of encapsulation, it often has more to do with A controlling state, and B with syntactic convenience for the caller.
To get some context, we first have to consider the programming language itself.
While many languages have "Properties", both terminology and the capabilities vary dramatically.
The C# programming language has a very sophisticated notion of properties on both the semantic and a syntactic level. It allows either the getter or setter to be optional, and it critically allows them to have different visibility levels. This makes a big difference if you wish to expose a fine grained API.
Consider an assembly that defines a set of data structures that are by nature cyclic graphs. Here is a simple example:
public sealed class Document
{
public Document(IEnumerable<Word> words)
{
this.words = words.ToList();
foreach (var word in this.words)
{
word.Document = this;
}
}
private readonly IReadOnlyList<Word> words;
}
public sealed class Word
{
public Document Document
{
get => this.document;
internal set
{
this.document = value;
}
}
private Document document;
}
Ideally, Word.Document would not be mutable at all, but I cannot express that in the language. I could have created an IDictionary<Word, Document> that mapped Words to Documents but ultimately there will be mutability somewhere
The intent here is to create a cyclic graph such that any function that is given a word can query the Word as to its containing Document by way of the Word.Document property (getter). We also want to prevent mutation by consumers after the Document has been created. The only purpose of the setter is to establish the link. It is only meant to be written once. This is hard to do because we have to create the Word instances first. By using C#'s ability to ascribe different levels of accessibility to the getter and setter of the same property, we are able to encapsulate the mutability of the Word.Document property within the containing assembly.
Code consuming these classes from another assembly will see the property as read-only (as only having a get not a set).
This example leads me to my favorite thing about properties. They can have a getter only. Whether you simply want to return a computed value or whether you want to expose only the ability to read but not to write, the use of properties is actually intuitive and simple for both the implementation and the consuming code.
Consider the trivial example of
class Rectangle
{
public Rectangle(double width, double height) => (Width, Height) = (width, height);
public double Area { get => Width * Height; }
public double Width { get; private set; }
public double Height { get; private set; }
}
Now, as I said previously, some languages have different concepts of what a property is. JavaScript is one such example. The possibilities are complex. There are a multitude of ways in which a property can be defined by an object and their are very different ways in which visibility can be controlled.
However, in the trivial case, as in C#, JavaScript allows properties to be defined such that they expose only a getter. This is wonderful for controlling mutation.
Consider:
function createRectangle(width, height) {
return {
get width() {
return width;
},
get height() {
return height;
}
};
}
So when should properties be avoided? In general, avoid setters that have logic. Even simple validation is often better accomplished elsewhere.
Going back to C#, it is often tempting to write code such as
public sealed class Person
{
public Address Address
{
get => this.address;
set
{
if (value is null)
{
throw new ArgumentException($"{nameof(Address)} must have a value");
}
this.address = value;
}
}
private Address address;
}
public sealed class Address { }
The exception thrown is likely a surprise to consumers. After all, Person.Address is declared mutable and null is a perfectly valid value for a value of type Address. To make matters worse, an ArgumentException is being raised. The consumer may not be aware that they are calling a function and so the type of the exception adds even more to the surprising behavior. Arguably, a different exception type, such as InvalidOperationException would be more appropriate. However, this highlights where setters can get ugly. The consumer is thinking of things in different terms. There are times when this sort of design is useful.
Instead, however, it would be better to make Address a required constructor argument, or create a dedicated method for setting it if we must expose write access, and expose a property with only a getter.
I will add more to this answer.
