Nested object to hierarchical object list

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 GetLayers()
        {
            return new List
                   {
                       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 ChildLayers { get; set; }

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

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

IList hierarcy = new List();

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);

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 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 GetLayers()
{
    IList data = Db.GetLayers();

    IList hierarcy = new List();

    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 ChildLayers { get; set; }

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

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 ChildLayers { get; private set; }

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

StackExchange Code Review Q#102389, answer score: 12