Why is encapsulation so often ignored?

Introduction

I started working at a new project (for me anyway, the project is in fact in development for some time) and came once again to see the some OOP design issues which unfortunately I do see very often. Any basic course in OOP introduces the core principles: encapsulation, abstraction, inheritance, polymorphism. Basic as they are, I see more classes in which they are not respected than classes in which they are. For some reasons these concepts are not internalized properly – it feels like everybody knows the lyrics by heart but few can make any reasoning about the story told by them. Let’s start with the first one, the encapsulation.

Encapsulation

Most of my life as a developer I used C# and I believe it is a great language – an obviously subjective opinion because of this. The one thing which I like and less and less are the properties. Not because they aren’t useful, because they are, but because they make it so easy to do something you shouldn’t do – break encapsulation. I won’t get in the details of how and why it is so since that is already explained elsewhere, like http://typicalprogrammer.com/doing-it-wrong-getters-and-setters/ or http://c2.com/cgi/wiki?AccessorsAreEvil . One can argue that properties, like everything else, can be used either in the proper or in the wrong way. True, but the terse syntax to define accessors and hence the very easy way to expose the internals of a class is, to some extent, making developers to believe it is not a bad idea to do so. The platform (programming language included) should make what is unlikely to be the proper to do hard to do, and what is likely the good thing do easy to do.

Let’s see an example. All the projects I have worked on had some settings which were per user (user selected language preference, time zone, theme, etc.) or some global settings (url of the server to be used, for ex) which made sense to be kept in the backend db. As a result each project had a class, say Settings which was used to retrieve and save those settings. The class looks as follows in most of the cases which I have seen:

public class Configuration
{
    public string Key { get; set; }
    public string Value { get; set; }
}
public class Settings
{
    public Configuration Get(string key)
    {
       Configuration record = null;
       //load the record from db
       return record;
    }
...
}

//the usage looks like this
var settings = new Settings();
var config = settings.Get("Timeout");
int timeout;
if(!int.TryParse(config.Value, out timeout))
{
    throw new InvalidOperationException("Timeout key not found");
}
//code which uses timeout variable
...

Why this code is far from good? The list is long but let’s start with encapsulation. The Settings class exposes information which it internal: that the data is stored in a key/value pair in which both are strings (the Configuration class) and that there is a key “Timeout”.

Breaking encapsulation is actually bad because makes the systems in which that happens hard to maintain. Why are hard to maintain? Rant starting: the primary force which has driven and shaped the software development (and the programming languages which are an important part of the process) is managing complexity. This is why we are not using machine code, or assembly, or C for that matter (well, most of us). So how can we manage complexity? By dividing complex problems into smaller, simpler ones, constructing abstractions which internally solve those problems and composing those abstractions to solve the initial problem. In OOP the class is used to implement both ways of managing complexity. The class should have one responsability which can be isolated (to some extent) from the rest of the system. When the need to change that functionality arises, if that class is well implemented then the developer should understand only that class to make the change, and not the whole system (not even the classes which depend on it, if the contract is not changed). When arguing why it should be this way one common argument is: if the underlying implementation has to change, say from using db store x to db store y, then it will be easy to do so. This argument undermines the actual reason, since every experienced developer knows it is unlikely for that to happen. Swapping one implementation for another is not the actual core reason, but if you can do it then you have the confirmation that the encapsulation has been respected. As I have mentioned before, the reason is to isolate the behavior defined in that class from the rest of the system in order to make it simple to understand and change that behavior. Rant over, let’s return to our classes.

Why is harder to maintain the code in this often found implementation? Say we have to add a new type of timeout – the first one specified the timeout for the db connection, but now we need one for the web server connection. We want to rename the first one as “WebTimeout” and to name the other “DbTimeout”. In this implementation we would have to search for all the usages of “timeout” (and this string could be used in many other places for different purposes) and make the changes. With this in mind, we change out Settings class:

public class Configuration
{
    public string Key { get; set; }
    public string Value { get; set; }
}
public class Settings
{
    public const string DbTimeout = "db_timeout";
    public const string WebTimeout = "web_timeout";
    public Configuration Get(string key)
    {
        Configuration record = null;
        //load the record from db
        return record;
    }
...
}
//usage
var settings = new Settings();
var config = settings.Get(Settings.DbTimeout);
int timeout;
if(!int.TryParse(config.Value, out timeout))
{
    throw new ArgumentOutOfRangeException(Settings.DbTimeout + " not found");
}
//code which uses timeout variable
...

Let's say another change is requested: if the value is not specified we want to use a default one. To implement this we have to search for every usage of the key and updating the code as follows:

...
var settings = new Settings();
var config = settings.Get(Settings.DbTimeout);
int timeout;
if(!int.TryParse(config.Value, out timeout))
{
    throw new ArgumentOutOfRangeException(Settings.DbTimeout + " not found");
}
else
{
    timeout = 1000;
}
//code which uses timeout variable
...

Similarly, if the default value changes than we have to search for all the places where the Settings.DbTimeout key is used and change it. There is a risk to miss some of those places, so to avoid that we can add a constant in the Settings class, say

public const int DefaultDbTimeout = 1000;

It is better, but we could go on and as other changes are requested we would have to go and make a change in every place where the keys are used. The bad design makes the classes using settings burden with the knowledge of its internals and any change to the Settings class will ripple through all of them. So how can we respect encapsulation? It is helpful to ask the following question: what is the minimum of information I have to pass on to get what I need and which is the response which best suits my needs? One typical answer looks like this:

public class Settings
{
    private const int DefaultDbTimeout = 1000;
    public const string DbTimeout = "db_timeout";
    public const string WebTimeout = "web_timeout";
    public int GetInt(string key)
    {
       string value = null;//GetFromDb(key).Value;//load the record from db
       int result;
       if (!int.TryParse(value, out result))
       {
           switch (key)
           {
               case DbTimeout:
                   result = DefaultDbTimeout;
                   break;
               //....some other cases
               default:
                   throw new ArgumentOutOfRangeException(key + " not found");
           }
        }
        return result;
    }
...
}

//usage
var settings = new Settings();
var timeout = settings.GetInt(Settings.DbTimeout);
//code which uses timeout variable
...

This is better – we no longer return the Configuration instance so that class can change without affecting the Settings's clients (classes using Settings instances) which was not adding any benefit anyway, we removed the duplicate code scattered in all the clients for all the keys, all the logic related to keys and values in one place – the Settings class. Have we hidden all the internals of the Settings class from its clients? Well, not really – they still know that it is using some string key/value storage. That can change too – say we want to optimize the queries and intead of the string key given as parameter we want to use the id of the record – we still are exposing internal logic when we shouldn’t. We can remove that information too:

public class Settings
{
    private const int DefaultDbTimeout = 1000;
    private const string DbTimeout = "db_timeout";
    private const string WebTimeout = "web_timeout";
    public int GetDbTimeout()
    {
        string value = GetFromDb(DbTimeout);
        int result;
        if (!int.TryParse(value, out result))
        {
            result = DefaultDbTimeout;
        }
        return result;
    }
    public int GetWebTimeout()
    {
        //actual implementation
        return 0;
    }
    private string GetFromDb(string key)
    {
        Configuration record = null;// GetFromDb(key);//load the record from db
        return record.Value;
    }
...
}
//usage
var settings = new Settings();
var timeout = settings.GetDbTimeout();
//code which uses timeout variable
...

The internal implementation of the Settings class is less important for this discussion – it can be improved. The important thing is that now we expose a method which will return the appropriate key value having the desired type.

Conclusion

This example is quite simple and common, but with all this I see it implemented more often like in the first approach and almost never like in the last one, even if written by a senior developer. I would have expected to encounter more subtle issues within the projects I came to work with, but it seems that even basic OOP design principles are poorly internalized and applied. I am interested in your experiences on the matter so please share them. Why do you believe this happens?