Representing relations between devices when presented with only one-to-many mappings

Question

There are devices that communicate between each other. Already existing in the Legacy codebase is a list of one-to-many mappings between the devices. I have written a function in my backend that processes this list. Each mapping is in the form of a custom object that stores the source object and the multiple destinations that the source is mapped to.

In the front-end, I have a list of devices as checkboxes. When I select a device, I need to find out for each device, all the other devices that it is related to, both, when acting as a sender, or and as a receiver. Then I need to check those boxes as well.

My approach so far has been to iterate over each Key-Value Pair in the custom mapping list, find the source, identify the destinations in the mapping, add this to a source-destination collection.

Then, iterate over this mapping list again. But this time, because the source-destination mapping is one-to-many, I find that getting a destination, adding it to a list and backtracking towards the entire list to find out sources can be quite inefficient once the sizes get large. But I am unable to think of a more efficient way to do the same.

The current code works. But freezes the UI for a few seconds when it runs, but I think the approach and/or the usage of Data Structures can be optimized. Readability and semantic correctness is what I am looking for.

public static List<Dictionary<string, List<string>>> GetDependentDeviceMasterSet(Dictionary<CustomDeviceStruct, List<dataTag>> externalMappingList)
{
    var sourceDestinationMappings = new Dictionary<string, List<string>>();
    var destinationSourceMappings = new Dictionary<string, List<string>>();           
    var mappings = new List<Dictionary<string, List<string>>>();      

    foreach (var externalMapping in externalMappingList)
    {
        var sourceName = externalMapping.Key.deviceName;
        if (sourceDestinationMappings.ContainsKey(sourceName))
        {
            foreach (var data in externalMapping.Value)
            {
                var destinationName = data.deviceName;
                if((sourceDestinationMappings[sourceName]).Contains(destinationName) == false)
                { 
                    (sourceDestinationMappings[sourceName]).Add(destinationName);
                    if (destinationSourceMappings.ContainsKey(destinationName))
                    {
                        foreach (var includedSourceName in destinationSourceMappings[destinationName])
                        {
                            if (destinationSourceMappings[destinationName].Contains(includedSourceName) == false)
                            {
                                destinationSourceMappings[destinationName].Add(includedSourceName);
                            }
                        }
                    }
                }                        
            }
        }
        else
        {
            var destinations = new List<string>();
            foreach (var data in externalMapping.Value)
            {
                var destinationName = data.deviceName;
                destinations.Add(destinationName);
                if (destinationSourceMappings.ContainsKey(destinationName))
                {
                    if (destinationSourceMappings[destinationName].Contains(sourceName) == false)
                    {
                        destinationSourceMappings[destinationName].Add(sourceName);
                    }
                }
                else
                {
                    var sources = new List<string>();
                    sources.Add(sourceName);
                    destinationSourceMappings.Add(destinationName, sources);                        
                }
            }
            sourceDestinationMappings.Add(sourceName, destinations);
        }
    }
    mappings.Add(sourceDestinationMappings);
    mappings.Add(destinationSourceMappings);
    return mappings;
}

And in the front-end, I process it like so :

private void FillAllNodesThatAreRelatedTo(TreeNode selectedDeviceNode)
{  
    foreach(TreeNode childNode in selectedDeviceNode.Parent.Nodes)
    {
        if (childNode.Text == selectedDeviceNode.Text)
        {
            continue;
        }

        if (this.dependentDeviceMasterSet[0].ContainsKey(selectedDeviceNode.Text))
        {
            var sourceDestinationMapping = this.dependentDeviceMasterSet[0][selectedDeviceNode.Text];
            if (sourceDestinationMapping.Contains(childNode.Text))
            {
                childNode.Checked = true;
                continue;
            }
            else
            {
                childNode.Checked = false;
                continue;
            }    
        }               

        if (this.dependentDeviceMasterSet[1].ContainsKey(selectedDeviceNode.Text))
        {
            var destinationSourceMapping = this.dependentDeviceMasterSet[1][selectedDeviceNode.Text];
            if (destinationSourceMapping.Contains(childNode.Text))
            {
                childNode.Checked = true;
                continue;
            }
            else
            {
                childNode.Checked = false;
                continue;
            }
        }
        if ((this.dependentDeviceMasterSet[0].ContainsKey(selectedDeviceNode.Text) == false) &&
            (this.dependentDeviceMasterSet[1].ContainsKey(selectedDeviceNode.Text) == false))
        {
            childNode.Checked = false;
            continue;
        }
    }
}

For the first line of code, using LINQ what I have tried so far to simplify it is:

var mappingsThatContainSource = from mapping in externalMappingList
                                where sourceDestinationMappings.ContainsKey(mapping.Key.deviceName)
                                select mapping;

But I am unsure if I am on the right track. Known issues:

1. Too much nesting. Hence, a want to try LINQ.
2. Inefficient use of Data Structures

Answer 1

(This is based on my answer from yesterday, so it is possible that not all names are completely in line with the ones presented above.)

---

A Dictionary<string, List<string>> I can understand, but a List<Dictionary<string, List<string>>> has my eye twitching uncontrollably. ;-)

Even Dictionary<CustomDeviceStruct, List<dataTag>> seems odd to me: why isn't List<dataTag> simply a property of CustomDeviceStruct?

---

Don't use ContainsKey, use TryGetValue. That way you also avoid repeatedly using devices like sourceDestinationMappings[sourceName].

---

I cannot figure out what this is supposed to do:

if (destinationSourceMappings.ContainsKey(destinationName))
{
    foreach (var includedSourceName in destinationSourceMappings[destinationName])
    {
        if (destinationSourceMappings[destinationName].Contains(includedSourceName) == false)
        {
            destinationSourceMappings[destinationName].Add(includedSourceName);
        }
    }
}

As I read it, it seems like you loop through all elements of a List<T> to check if each element of that list is present in the list, and if it isn't (but why would that even be possible?) you add it to the list.

---

Let's try to rewrite your logic, which unfortunately intertwines various processes. Starting with the else, which can be simplified to this:

var destinations = externalMapping.Value.Select(x => x.Name).ToList();

foreach (var dataTag in externalMapping.Value)
{
    var destinationName = dataTag.Name;
    if (!destinationSourceMappings.TryGetValue(destinationName, out List<string> sources))
    {
        sources = new List<string>();
    }

    sources.Add(sourceName);
    destinationSourceMappings[destinationName] = sources.Distinct().ToList();
}

sourceDestinationMappings.Add(sourceName, destinations);

But that still seems cluttered and too elaborate for what it does. Let's rework that some more, for instance by moving the destinations assignation to the end and some other small improvements:

foreach (var destinationName in externalMapping.Value.Select(x => x.deviceName))
{
    if (!destinationSourceMappings.TryGetValue(destinationName, out List<string> sources))
    {
        sources = new List<string>();
    }

    sources.Add(sourceName);
    destinationSourceMappings[destinationName] = sources.Distinct().ToList();
}

sourceDestinationMappings.Add(sourceName,
    externalMapping.Value.Select(x => x.deviceName).ToList());

Quite frankly, this still doesn't satisfy me. It feels inelegant. But at least it is much more readable that the original, because you don't need to parse 20+ lines to discover that two separate processes are happening inside that else.

BTW if sources was a HashSet, you wouldn't even need to do sources.Distinct().ToList();

---

When looking at the if and the logic as a whole, I found that it could be rewritten as follows:

var sourceDestinationMappings = externalMappingList
            .ToDictionary(x => x.Key.deviceName, 
                x => x.Value.Select(dataTag => dataTag.deviceName).ToList());

foreach (var externalMapping in externalMappingList)
{
    var sourceName = externalMapping.Key.deviceName;

    if (!sourceDestinationMappings.TryGetValue(sourceName, out List<string> destinations))
    {
        destinations = new List<string>();

        foreach (var destinationName in externalMapping.Value.Select(x => x.deviceName))
        {
            if (!destinationSourceMappings.TryGetValue(destinationName, out List<string> sources))
            {
                sources = new List<string>();
            }

            sources.Add(sourceName);
            destinationSourceMappings[destinationName] = sources.Distinct().ToList();
        }
    }

    destinations.AddRange(externalMapping.Value.Select(x => x.deviceName));
    sourceDestinationMappings[sourceName] = destinations.Distinct().ToList();
}

But when I then stepped through the logic, I found that this still does a number of things that don't seem necessary. And I distilled it down to this:

var sourceDestinationMappings = externalMappingList
    .ToDictionary(x => x.Key.deviceName,
        x => x.Value.Select(dataTag => dataTag.deviceName).ToList());

foreach (var externalMapping in externalMappingList)
{
    var sourceName = externalMapping.Key.deviceName;

    foreach (var destinationName in externalMapping.Value.Select(dataTag => dataTag.deviceName))
    {
        if (!destinationSourceMappings.TryGetValue(destinationName, out List<string> sources))
        {
            sources = new List<string>();
        }

        sources.Add(sourceName);
        destinationSourceMappings[destinationName] = sources.Distinct().ToList();
    }
}

---

To be honest, I am not sure this solution is 100% correct. You would need to test this yourself: take the same dataset and apply both your code and this code and see if the results match for the various scenarios. And even if they do, don't just copy-paste what I wrote: try to follow what changes I made and why I did so.

Compare for instance the last two code blocks: look at destinations, and notice how in the earlier version nothing happens to it that cannot be scrapped. - We take the Value of sourceDestinationMappings, - we then add externalMapping.Value.Select(x => x.deviceName) to the existing destinations, - and then we remove the duplicate entries when we assign destinations back to sourceDestinationMappings[sourceName].

But all of that already happened in the first line:

var sourceDestinationMappings = externalMappingList
            .ToDictionary(x => x.Key.deviceName, 
                x => x.Value.Select(dataTag => dataTag.deviceName).ToList());

But externalMappingList hasn't changed since then, so all of that logic is pointless. And thus you can remove it, which means you end up with the final version.

---

As for the UI code you included here:

• Retrieve selectedDeviceNode.Text once and store it in a clear variable name if you're going to reuse it several times.
• Don't use ContainsKey when you need to use the result if the key is present. Even worse: you're using ContainsKey twice!
• The same logic is applied to dependentDeviceMasterSet[0] and dependentDeviceMasterSet[1], so this obviously should have been moved to a separate method.

And then you end up with something like this:

private void FillAllNodesThatAreRelatedTo(TreeNode selectedDeviceNode)
{  
    foreach(TreeNode childNode in selectedDeviceNode.Parent.Nodes)
    {
        if (childNode.Text == selectedDeviceNode.Text)
        {
            continue;
        }

        var sourceContains = CheckDevice(childNode, 0);
        var destinationContains = CheckDevice(childNode, 1);

        if (!sourceContains && !destinationContains)
        {
            childNode.Checked = false;
        }
    }
}

private bool CheckDevice(TreeNode childNode, int setId)
{
    if(this.dependentDeviceMasterSet[setId].TryGetValue(childNode.Text, out List<string> mapping)
    {
        childNode.Checked = mapping.Contains(childNode.Text);
        return true;
    }

    return false;
}

This wasn't tested, this was written in Notepad++. Look at it as an example of reusing code.

(Also, what kind of front-end is this code for? ASP.NET WebForms? Isn't there a better way to bind a grid to data instead of looping through something?)