Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

4 Answers 4

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 I 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 e. 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.


share|improve this answer

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.

share|improve this answer

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" }
        };
    }
}
share|improve this answer

If you wanna work with LINQ, probably the code below could help you:

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

            data.Where(x => x.ParentId == 0).ToList().ForEach(x => hierarcy.Add(x)); //get parent

            data.Where(a => a.ParentId != 0).ToList().
                ForEach(a => 
                {
                    hierarcy.Where(b => b.Id == a.ParentId).ToList().ForEach(c => c.ChildLayers.Add(a)); //get childrens
                });

            return hierarcy;
        }
share|improve this answer
1  
How can it help make the code better? This is just a code dump, and will get downvoted quickly. please explain how it would make the OP's code better. –  Malachi 17 hours ago
    
Thanks for your advice. But as you can read, He has put this "But this LayerService code is ugly. Is there any practical way with Linq or else? Is there a cleaner way?". So, if he needs Linq, he now has Linq. –  Jotch 1 hour ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.