1
\$\begingroup\$

I'm new to C# (coming from a JavaScript background) and it seems like this code could be greatly improved.

This SQL query:

SELECT RegionString,SubRegionString,CountryString,COUNT(*) AS size
FROM table
GROUP BY RegionString,SubRegionString,CountryString

Returns:

RegionString SubRegionString CountryString Size
-----------------------------------------------
Americas                                  2
Americas   NorthAmerica    Canada         5
Americas   NorthAmerica    US             3
Americas   SouthAmerica    Chile          3
EMEA       AsiaPacific     Australia      2
EMEA       AsiaPacific     Japan          1
EMEA       SouthernEurope  Turkey         1
EMEA       WesternEurope                  1

I made this C# code:

public class NameChildObject
{
    public string name { get; set; }
    public int size { get; set; }
    public List<NameChildObject> children { get; set; }

    public NameChildObject() 
    {
        children = new List<NameChildObject>();
    }
}

public ActionResult ByRegion()
{
    var returnResults = new List<NameChildObject>();

    var uniqueRegions = (from row in repository.GetAllEntities()
                         select row.RegionString).Distinct();

    foreach (string region in uniqueRegions)
    {
        returnResults.Add(new NameChildObject() { name = region });

        var uniqueSubRegions = (from row in repository.GetAllEntities()
                                where row.RegionString == region
                                select row.SubRegionString).Distinct();

        foreach (string subRegion in uniqueSubRegions)
        {
            var regionObject = returnResults.Find(row => row.name == region);

            var countryInfo = (from row in repository.GetAllEntities()
                               where row.SubRegionString == subRegion
                               group row by row.CountryString into g
                               select new NameChildObject() { name = g.Key, size = g.Count() });

            regionObject.children.Add(new NameChildObject() { name = subRegion, children = countryInfo.ToList()});
        }
    }

    return Json(returnResults, JsonRequestBehavior.AllowGet);
}

To convert the data into this format:

[
    {
        "name": "Americas",
        "size": 0,
        "children": [
            {
                "name": "",
                "size": 0,
                "children": [
                    {
                        "name": "",
                        "size": 2,
                        "children": []
                    }
                ]
            },
            {
                "name": "NorthAmerica",
                "size": 0,
                "children": [
                    {
                        "name": "Canada",
                        "size": 5,
                        "children": []
                    },
                    {
                        "name": "US",
                        "size": 3,
                        "children": []
                    }
                ]
            },
            {
                "name": "SouthAmerica",
                "size": 0,
                "children": [
                    {
                        "name": "Chile",
                        "size": 3,
                        "children": []
                    }
                ]
            }
        ]
    },
    {
        "name": "EMEA",
        "size": 0,
        "children": [
            {
                "name": "AsiaPacific",
                "size": 0,
                "children": [
                    {
                        "name": "Australia",
                        "size": 2,
                        "children": []
                    },
                    {
                        "name": "Japan",
                        "size": 1,
                        "children": []
                    }
                ]
            },
            {
                "name": "SouthernEurope",
                "size": 0,
                "children": [
                    {
                        "name": "Turkey",
                        "size": 1,
                        "children": []
                    }
                ]
            },
            {
                "name": "WesternEurope",
                "size": 0,
                "children": [
                    {
                        "name": "",
                        "size": 1,
                        "children": []
                    }
                ]
            }
        ]
    }
]
\$\endgroup\$
3
\$\begingroup\$

You have three calls to repository.GetAllEntities(). If those execute a db call each time, that's a bad thing. But it actually points to a deeper issue.

May I introduce you to Enumerable.GroupBy:

foreach(var regionStringGroup in repository.GetAllEntities().GroupBy(x => x.RegionString))
{
    var regionObject = new NameChildObject() { name = region };

    foreach(var subRegionStringGroup in regionStringGroup.GroupBy(x => x.SubRegionString))
    {
        var children = subRegionStringGroup
           .GroupBy(x => x.CountryString)
           .Select(x => new NameChildObject() { name = x.Key, size = x.Count() }).ToList()
        regionObject.children.Add(new NameChildObject() 
           { name = subRegion, children = children});
    }

    returnResults.Add(regionObject);
}

I do have to say I'm not 100% sure about how you create countryInfo because I lack some knowledge about the data you're processing, so please don't just use my code blindly but compare it to your results.


children, name, size: public properties should be PascalCase.


Is your class actually called NameChildObject? Can't say I like that name, especially the "Object" at the end.

Ditto for the property RegionString by the way: avoid suffixing a name with its type. Ditto CountryString.

ByRegion() isn't a good method name. GetByRegion doesn't feel right either since the "by region" implies there's a parameter, and there isn't. Perhaps something like GetRegionTree?

Size isn't really the best name either. Perhaps ChildCount? I don't even think it should be part of the query, since you could simply count the items in your various groups you're not using it anyway.


Hmm, I just noticed the resulting JSON is quite odd. Why not include the Size when you're constructing the RegionString-level NameChildObject?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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