Common Refactors: Example 2.

(Introduction to this series here)

This is one of my favorite code smells.

public List<Device> FilterDevices(List<Device> devices, 
                                   string name, 
                                   string serial, 
                                   string version)
{
    var result = new List<Device>();

    if (name != null && serial != null && version != null)
    {
        result = devices
            .Where(s => s.Name == settings.profile.name)
            .Where(s => s.Serial == settings.hardware.serialNumber)
            .Where(s => s.Version == settings.firmware.version)
            .ToList();
    }
    else if (name != null && serial != null)
    {
        result = devices
            .Where(s => s.Name == settings.profile.name)
            .Where(s => s.Serial == settings.hardware.serialNumber)
            .ToList();
    }
    else if (name != null && version != null)
    {
        result = devices
            .Where(s => s.Name == settings.profile.name)
            .Where(s => s.Version == settings.firmware.version)
            .ToList();
    }
    else if (serial != null && version != null)
    {
        result = devices
            .Where(s => s.Serial == settings.hardware.serialNumber)
            .Where(s => s.Version == settings.firmware.version)
            .ToList();
    }
    else if (name != null)
    {
        result = devices
            .Where(s => s.Version == settings.firmware.version)
            .ToList();
    }
    else if (serial != null)
    {
        result = devices
            .Where(s => s.Serial == settings.hardware.serialNumber)
            .ToList();
    }
    else if (version != null)
    {
        result = devices
            .Where(s => s.Version == settings.firmware.version)
            .ToList();
    }

    return result;
}

Oh lord.

This function checks the presence of every combination of every parameter provided. Imagine what happens if we add another two more.
How many tests would we need to ensure that this works? Or even more worrying, who would notice if it didn’t?

This reeks like the type of bug that slips into production.

So, let’s get started!

Is it doing what it should?

I’ll spare you the test suite, but yes.

Is it doing things it shouldn't?

No. The function copies a subset of a list based on a set of queries, which is desired.

Is it readable?

I mean, it’s not cryptic. It is overtly verbose though, and riddled with footguns.

Is it prone to exploding?

Yes.

Let's simplify it.

A little functional programming never hurt nobody.

Instead of providing a large amount of parameters and arbitrarily null checking various of combinations of them, lets send a list of predicate functions that can be reduced via Any(), which returns if any one of them pass.

public List<Device> FilterDevices(List<Device> devices, 
                                   List<Func<Device, bool>> predicates)
{
    return devices.Where(device => 
        predicates.Any(predicate => predicate(device)))
        .ToList();
}

Here it is in use:

var predicates = new List<Func<Device, bool>>()
{
    d => d.Name == settings.profile.name,
    d => d.Version == settings.firmware.version,
};

var filtered = FilterDevices(devices, predicates);

Now we can supply whatever number of queries when we need them, in any amount, without having to know parameters in advance. This means that we don’t have to null check an ever increasing number of variable combinations.

Also note that this function can now be reused, as we moved the values to be checked against to be provided by the caller.

Removing code is a wonderful feeling.

< Previous

Next >