Eliminate barely used fields


Let’s start this blog with something simple but that I see every once in a while during code reviews: barely used fields.

These are fields that get used once in a while, but not all the time. In the middle of all your code they seem to be necessary, but closer inspection may reveal that they are really not needed. Their usage is usually surrounded with null checks repeated all over the place.

For example, this week I reviewed some code that resembled this:

class SomeClass
{
    private IDictionary<K,V> userData;
    private IDictionary<K,V> systemData;

    SomeClass()
    {
        this.userData = codeToFindUserData();
        if (this.userData == null)
        {
            this.systemData = codeToFindSystemData();
        }
    }

    V GetSomeData(K key)
    {
        V value = someDefaultValueForAllKeys;

        if (this.userData != null)
        {
           value = this.userData[key];
        }
        else if (this.systemData != null)
        {
            value = this.systemData[key];
        }

        return value;
    }
}

In the code above we have the two dictionaries holding settings that apply to the current user and to the system. When getting data out of the class, settings from the user override settings from the system and these override any default value hardcoded in the class.

At no particular time we have a need to read data from both the user and system dictionaries, so it is clearly unnecessary to keep both fields in the class when only one is ever going to be used. The class can be modified to keep only the most important source:

class SomeClass
{
    private IDictionary<K,V> data;

    SomeClass()
    {
        this.data = codeToFindUserData() ?? codeToFindSystemData();
    }

    V GetSomeData(K key)
    {
        V value = someDefaultValueForAllKeys;

        if (this.data != null)
        {
           value = this.data[key];
        }

        return value;
    }
}

Depending on the scenario one may even go one step further and create a dictionary with the default values:

class SomeClass
{
    private static IDictionary<K,V> defaultValues = codeToCreateDefaultValues();
    private IDictionary<K,V> data;

    SomeClass()
    {
        this.data = codeToFindUserData() ?? codeToFindSystemData() ?? defaultValues;
    }

    V GetSomeData(K key)
    {
       return this.data[key];
    }
}

This type of pattern appears with data from several places. In our particular case, user and system data were read from the Active Directory; you can imagine similar situations with registry keys, configuration files and other types of data.

As long as the data only needs to be read from a single place at any given time, there’s no need to keep multiple fields around and juggle them with multiple if statements. Make the decision once during initialization and then reuse the results whenever needed.

Comments (0)

Skip to main content