Nested object to hierarchical object list

Question

I have a C# type to generate Layer SubLayer:

public class Layer
{
    public int Id { get; set; }
    public int ParentId { get; set; }
    public string Name { get; set; }
}

And simulated a list with a Db class:

public static class Db
    {
        public static IList<Layer> GetLayers()
        {
            return new List<Layer>
                   {
                       new Layer{Id = 1, ParentId = 0, Name = "First Layer" },
                       new Layer{Id = 2, ParentId = 1, Name = "First SubLayer1" },
                       new Layer{Id = 3, ParentId = 1, Name = "First SubLayer2" },
                       new Layer{Id = 4, ParentId = 1, Name = "First SubLayer3" },
                       new Layer{Id = 5, ParentId = 0, Name = "Second Layer" },
                       new Layer{Id = 6, ParentId = 5, Name = "Second SubLayer1" },
                       new Layer{Id = 7, ParentId = 5, Name = "Second SubLayer2" },
                       new Layer{Id = 8, ParentId = 7, Name = "Sub -3" }
                   };
        }
    }

I want to get a serialized JSON object, so I created a service class to generate and changed the Layer class:

public class Layer
{
    public int Id { get; set; }
    public int ParentId { get; set; }
    public string Name { get; set; }
    public IList<Layer> ChildLayers { get; set; }

    public Layer()
    {
        ChildLayers = new List<Layer>();
    }
}

public class LayerService
{
    public IList<Layer> GetLayers()
    {
        IList<Layer> data = Db.GetLayers();

        IList<Layer> hierarcy = new List<Layer>();

        foreach (var layer in data)
        {
            var layer1 = layer;

            var sublayers = data.Where(i => i.ParentId == layer1.Id && i.ParentId !=0);

            var enumerable = sublayers as Layer[] ?? sublayers.ToArray();

            if(enumerable.Any() && layer.ParentId ==0)
                hierarcy.Add(layer);

            foreach (var sublayer in enumerable)
            {
                layer.ChildLayers.Add(sublayer);    
            }
        }

        return hierarcy;
    }
}

But this LayerService code is ugly. Is there any practical way with Linq or else? Is there a cleaner way?

Answer 1

var sublayers = data.Where(i => i.ParentId == layer1.Id && i.ParentId !=0);  

you should let the operators some room to breathe. Adding a space after = makes your code more readable.

Bug alert

In the above query you are filtering for i.ParentId != 0 and later on you have the condition && layer.ParentId ==0 so your hierarchy wil remain empty.

---

var enumerable = sublayers as Layer[] ?? sublayers.ToArray();  

This makes me just wonder. What sense does it have to convert the IEnumerable<Layer> to an array if the next thing you do is iterating over the items by using a foreach loop ?

var layer1 = layer;  

this isn't necessary at all, it is sufficient to use the layer inside linq expression.

---

You should always use braces {} for single command if statements to make your code less error prone. If you decide to no use them, please don't come back whining that you have a bug you can't find (humour btw.).

---

Integrating the mentioned points will lead to

public IList<Layer> GetLayers()
{
    IList<Layer> data = Db.GetLayers();

    IList<Layer> hierarcy = new List<Layer>();

    foreach (var layer in data)
    {

        var sublayers = data.Where(i => i.ParentId == layer.Id && i.ParentId != 0);

        if (sublayers.Any())
        {
            hierarcy.Add(layer);
        }
        foreach (var sublayer in sublayers)
        {
            layer.ChildLayers.Add(sublayer);
        }
    }

    return hierarcy;
}  

---

public class Layer
{
    public int Id { get; set; }
    public int ParentId { get; set; }
    public string Name { get; set; }
    public IList<Layer> ChildLayers { get; set; }

    public Layer()
    {
        ChildLayers = new List<Layer>();
    }
}  

You shouldn't expose the ChildLayers property setter to the public. If someone comes along setting it to null you would be lost.

You should just change it to

public IList<Layer> ChildLayers { get; private set; }  

Otherwise using autoimplemented properties, as long as you don't need validation in the setters , is the way to go.

---

Answer 2

Inside your foreach statement, there is no reason to assign the iterator variable to another variable. I don't see where it helps you any. The only place you use that variable is when assigning the sublayers variable, and you could just use the iterator to do that anyway.

Answer 3

There is also a recursive solution:

public IList<Layer> GetLayers()
{
    //call it with parentId=0 initially, to get parentless nodes
    return GetChildren(Db.GetLayers(), 0);
}

private IList<Layer> GetChildren(IList<Layer> source, int parentId)
{
    var children = source.Where(x => x.ParentId == parentId).ToList();
    //GetChildren is called recursively again for every child found
    //and this process repeats until no childs are found for given node, 
    //in which case an empty list is returned
    children.ForEach(x => x.ChildLayers = GetChildren(source, x.Id));
    return children ;
}

You should make sure, that you do not have circular dependencies though. Otherwise you might run into StackOverflowException.

P.S. You probably need to use two separate classes here. One to represent the data format you use to store your layers in DB:

//probably should not be public
public class LayerData
{
    public int Id { get; set; }
    public int ParentId { get; set; }
    public string Name { get; set; }
}

And another class to represent the layer model you are going to work with in your code:

public class LayerModel
{
    public int Id { get; set; }
    public string Name { get; set; }
    public LayerModel ParentLayer { get; set; }
    public IList<LayerModel> ChildLayers { get; set; }

    public Layer()
    {
        ChildLayers = new List<Layer>();
    }
}

You should not mix them.

Answer 4

There is still one thing that I don't like about your code that is not mentioned in the other answers namely the name of the Db.GetLayers method. If you are creating something new and your are, you should name the method appropriately like CreateLayers. If someone says GetSomething it means to me that this something already exists and this method retrievs it.

If this is however a test code you should also construct it to look like a debug/test code, for example:

public static class Db
{
    public static IList<Layer> GetLayers()
    {
#if DEBUG
        return CreateTestLayers();
#endif
        reutrn layers form db...

    }

    [Conditional("DEBUG")]
    public static IList<Layer> CreateTestLayers()
    {
        return new List<Layer>
        {
           new Layer{Id = 1, ParentId = 0, Name = "First Layer" },
           new Layer{Id = 2, ParentId = 1, Name = "First SubLayer1" },
           new Layer{Id = 3, ParentId = 1, Name = "First SubLayer2" },
           new Layer{Id = 4, ParentId = 1, Name = "First SubLayer3" },
           new Layer{Id = 5, ParentId = 0, Name = "Second Layer" },
           new Layer{Id = 6, ParentId = 5, Name = "Second SubLayer1" },
           new Layer{Id = 7, ParentId = 5, Name = "Second SubLayer2" },
           new Layer{Id = 8, ParentId = 7, Name = "Sub -3" }
        };
    }
}

Answer 5

Is there a cleaner way?

Make an intermediate structure that will help group the "flat" list of Layers coming from DB.

---

*Show me your data structures, and I won't usually need your code; it'll be obvious." - Fred Brooks, The Mythical Man Month

---

public Dictionary<int, List<Layer>> rackNstack = new Dictionary<int, List<Layer>>();

public List<Layer> BuildLayerHiArky(List<Layer> flatList) {
    rackNstack.Clear();

    foreach (Layer thing in flatList) {
        if(! rackNstak.ContainsKey(thing.ParentId)
            rackNstack.Add(thing.id, new List<Layer>());

        rackNstack[thing.ParentId].Add(thing);
    }

    List<Layer> allLayers = new Layer();
    Layer oneLayer;

    foreach (var layerList in rackNstack) {
        oneLayer = new Layer();
        oneLayer.Children == layerList.Value;
        oneLayer.Id = layerList.Key
        allLayers.Add(oneLayer);
    }
    return allLayers;
}