Common Refactors: Example 3.

(Introduction to this series here)

Time for a fun one!

This one has a larger set of tools that can be used to improve it, and thus provides an interesting discussion in which set of tools provides the most benefit.

public static List<StringTuple> PropertyAndValueFromElements
    (List<Element> elements)
{
    var result = new List<StringTuple>();

    foreach (var element in elements)
    {
        result.Add((
            element
                .Parent
                .Parent
                .Parent
                .Parent
                .Parent["property"] + ">Property",
            element
                .Parent
                .Parent["value"]
        ));
    }
    
    return result;
}

Is it doing what it should?

The six layer deep data structure is generated at runtime, so even though the answer right now might be yes, the question is for how long.

It assumes that all the children are reachable, which smells like a runtime error waiting to happen.

Is it doing things it shouldn't?

Because this function has a somewhat arbitrary way of extracting data, I’m not completely comfortable with also operating on several items at once.

Perhaps it would be better to extract the nested-parent-extraction part from the iterating part, and leave the latter for inlined mapping elsewhere.

Is it readable?

It’s a short function with no branches, but this is without error safety. As we’ll see, with the current implementation, this function will become less and less readable once we introduce safety precautions.

Is it prone to exploding?

Quite.

Let's introduce some safety.

Let’s start with adding null checks to the existing implementation.

if (element.Parent == null) {
    continue;
}

if (element.Parent.Parent == null) {
    continue;
}

if (element.Parent.Parent.Parent == null) {
    continue;
}

if (element.Parent.Parent.Parent.Parent == null) {
    continue;
}

if (element.Parent.Parent.Parent.Parent.Parent == null) {
    continue;
}

if (!element.Parent.Parent.Parent.Parent.Parent.HasValue("name")) 
{
    continue;
}

Ok, yeah, no.

Let’s do it implicitly with null-conditional member access:

if (!element.Parent?.Parent?.Parent?.Parent?.Parent?.HasValue("name")) 
{
    continue;
}

This is way shorter, but I don’t like it.

Another way would be to cover the entire operation in try-catch. This might be fine, but I find that a lot of try-catch patterns that wrap these type of uncertain operations tend to be used as crutches to mask a problem without actually solving it. I’d speculate that for this function such an approach would end up something like so:

foreach (var element in elements)
{
    try {
        result.Add((
            element
                .Parent
                .Parent
                .Parent
                .Parent
                .Parent["property"] + ">Property",
            element
                .Parent
                .Parent["value"]
        ));
    }
    catch (Exception e) {
        // You'd think there'd be code in here.
    }
}

So, how do we solve this problem declaratively, whilst minimizing potential bugs? Yet again, functional programming got our backs.

(Ideally, we’d want to change the underlying data structure, but for this instance we’re forced to stay with the current one)

Let’s start by writing a helper function to let us “dig” into our data structure and extract a value:

public static string GetElementValue(Element e, string key, int depth)
{
    if (e.Parent != null && depth > 0)
    {
        return GetElementValue(e.Parent, key, depth - 1);
    }

    // Iterative approach:
    // while (current.Parent != null && depth > 0) {
    //     current = current.Parent;
    //     depth--;
    // }

    if (e.ContainsKey(key)) {
        return e[key];
    }

    return $"Not found: {key}";
}

Now we can specify which value we want from which depth, without having to check for errors or worry about (practically invisible) one-off errors.

On top of that, the API for this function is quite readable:

GetElementValue(element: element, key: "property", depth: 5);

Let’s wrap the two calls in a decorator to make them explicit:

public static (string, string) PropertyAndValue(Element e) =>
    (GetElementValue(e, "property", 5) + ">Property",
     GetElementValue(e, "value", 2));

Now we can use this function to map over a list of elements.

Here it is in use:

var propertiesAndValues = 
    elements.Select(PropertyAndValue);

propertiesAndValues is the result of mapping PropertyAndValue over elements. To me, that is very readable.

By breaking this problem into a smaller set of functions, it is now much easier to extend and test this functionality without having to modify (and therefore potentially break) existing implementations.

Win-win.

< Previous