2
\$\begingroup\$

I have a collection (Child Collection) on which I am performing some operation to make another collection (Parent Collection). But that I do in a dirty foreach loop. I want to do it in an elegant way.

GroupInfo grpInfo;
List<GroupInfo> lstGroupInfo = new List<GroupInfo>();
foreach (AddressInfo addressInfo in subDetails)
{
 if (lstGroupInfo.Where(u => u.Addres1 == addressInfo.Address1).Count() > 0)
    {
        grpInfo = lstGroupInfo.Where(u => u.Addres1 == addressInfo.Address1).SingleOrDefault();                                  

        if (addressInfo.Rural)
            grpInfo.Rural = true;
        else if (addressInfo.Urban)
            grpInfo.Urban = true;

        grpInfo.SubDetails.Add(addressInfo);
    }
    else
    {
        grpInfo = new GroupInfo();    
        grpInfo.AddressID = addressInfo.AddressID;
        grpInfo.LocationID = addressInfo.NamedLocationID;

        if (addressInfo.Rural)
            grpInfo.Rural = true;
        else if (addressInfo.Urban)
            grpInfo.Urban = true;

    }
}

GroupInfo class is:

public class GroupInfo
{
    public string Address1
    {
        get;
        set;
    }

    public int AddressID
    {
        get;
        set;
    }

    public int? LocationID
    {
        get;
        set;
    }
}

I want to do it in LINQ lambda way.

\$\endgroup\$
2
  • \$\begingroup\$ You need to tidy up the code sample, it's very difficult to figure out what some of the variables are. For example, what type is lstGroupInfo? or tripAddressInfo? \$\endgroup\$
    – Quango
    Jan 18, 2012 at 14:11
  • \$\begingroup\$ Thanks for analyzing the query, i have updated the code. Kindly tell me if i could help me more. \$\endgroup\$
    – MegaMind
    Jan 19, 2012 at 4:06

1 Answer 1

1
\$\begingroup\$

There's quite a lot of logic in there, so I doubt you will get there completely by using LINQ, and, more importantly, if it would actually be more elegant.

I would rewrite as follows, and be done with it:

        foreach (AddressInfo addressInfo in subDetails)
        {
            grpInfo = lstGroupInfo.SingleOrDefault(x => x.Address1 == addressInfo.Address1);
            if (grpInfo != null)
            {
                grpInfo.Rural = addressInfo.Rural;
                grpInfo.Urban = addressInfo.Urban;
                grpInfo.SubDetails.Add(addressInfo);
            }
            else
            {
                grpInfo = new GroupInfo();
                grpInfo.AddressID = addressInfo.AddressID;
                grpInfo.LocationID = addressInfo.NamedLocationID;
                grpInfo.Rural = addressInfo.Rural;
                grpInfo.Urban = addressInfo.Urban;
            }
        }

In addition, it seems to me that Rural and Urban are mutually exclusive, so why not define an enumeration that contains those two (or more) values, or declare one boolean property IsRural to indicate if it's "Rural", and if not, it's urban. That would bring the code down to:

        foreach (AddressInfo addressInfo in subDetails)
        {
            grpInfo = lstGroupInfo.SingleOrDefault(x => x.Address1 == addressInfo.Address1);
            if (grpInfo != null)
            {
                grpInfo.IsRural = addressInfo.IsRural;
                grpInfo.SubDetails.Add(addressInfo);
            }
            else
            {
                grpInfo = new GroupInfo();
                grpInfo.AddressID = addressInfo.AddressID;
                grpInfo.LocationID = addressInfo.NamedLocationID;
                grpInfo.IsRural = addressInfo.IsRural;
            }
        }
\$\endgroup\$
5
  • \$\begingroup\$ Shouldnt the first if-clause state if(grpInfo != null) ? It will now generate an NullReferenceException if grpInfo actually is null. Good refactoring though, makes it much more readable \$\endgroup\$
    – Mattias
    Jan 19, 2012 at 8:12
  • \$\begingroup\$ Thanks Willem, but this is not exactly as expected, if you look at my more carefully, I intentially write IF condition on IsRural and IsUrban property, in your scenario it will be true or false as per the last addressInfo, but i want it to remains true if it is set to true once through out the process. \$\endgroup\$
    – MegaMind
    Jan 19, 2012 at 8:46
  • \$\begingroup\$ @Manvinder: MaybeI'm missing something, but I think yours will also reflect the state of the last AddressInfo in the loop. \$\endgroup\$ Jan 19, 2012 at 8:55
  • \$\begingroup\$ yes but it will update only if the value is true and not false \$\endgroup\$
    – MegaMind
    Jan 19, 2012 at 9:14
  • \$\begingroup\$ @Manvinder: Ah yes, I see. Then you'd need to bring those checks back in. It also leads to the second part of my answer: Why is there a Rural and Urban property? They seem mutually exclusive (can they both be false btw?). An enumeration or one boolean property seems more appropriate. But I don't have the design, nor the requirements, so I can't really tell. \$\endgroup\$ Jan 19, 2012 at 9:20

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.