Comparing two lists

Question

I am getting two different lists of members, then if certain conditions meet, I add more members to the list before converting into an array.

Is there any way this could be improve? I guess we can use Linq and cast but I am not advanced in either of the skills I mentioned.

    List<Member> Members = new List<Member>();
    foreach (SPListItem mItem in GetList(Url).Items)
    {
        Member m = new Member();
        m.ID = mItem.ID;
        m.Name = mItem.Title;
        m.Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]);
        m.eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]);
        m.Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]);
        m.Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol]);
        Members.Add(m);
    }

    if (DateTime.Now < row.EndDate)
    {
        var cd = new MemberManager().GetMoreMembers(Url + "/");
        var activeMembers = cd.Where(am => am.MembershipStatus == "Active" || am.MembershipStatus == "Pending").ToList();
        if (activeMembers != null || activeMembers.Count() > 0)
        {
            foreach (var am in activeMembers)
            {
                if (!Members.Any(a => a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant()))
                {
                    Member m = new Member();
                    m.Name = am.FirstName + " " + am.LastName;
                    m.eMail = am.Email;
                    m.IsVip = true;
                    Members.Add(m);
                }
            }
        }
    }

    md.Members = Members.ToArray();

Answer 1

You can shorten the following snippet by using LINQ and a projection:

List<Member> Members = new List<Member>();
foreach (SPListItem mItem in GetList(Url).Items)
{
    Member m = new Member();
    m.ID = mItem.ID;
    m.Name = mItem.Title;
    m.Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]);
    m.eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]);
    m.Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]);
    m.Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol]);
    Members.Add(m);
}

becomes

List<Member> Members = GetList(Url).Items.Select(item => new Member { 
                                                        ID = item.ID, 
                                                        Name = item.Title}).ToList();  
// Do this for every field you're interested in

---

var cd = new MemberManager().GetMoreMembers(Url + "/");

If a method doesn't require instance-level information, you should make it static. It will save you another object allocation and it just makes more sense to write MemberManager.GetMoreMembers() then.

---

am.MembershipStatus == "Active" || am.MembershipStatus == "Pending"

This would make more sense as an enum rather than a string. There's only a limited amount of values that status can be.

---

if (activeMembers != null || activeMembers.Count() > 0)

Boolean logic! You mean to use

if (activeMembers != null && activeMembers.Count() > 0)

---

a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant()

This will create 2 new string objects every iteration. Instead use string.Equals(a.eMail, am.Email, StringComparison.InvariantCultureIgnoreCase);

Notice also the eMail and Email discrepancy.

---

You can also shorten the above block by using

var newMembers = activeMembers.Where(activeMember => 
                    !Members.Any(member => string.Equals(activeMember.eMail, 
                                                          member.Email, 
                                                          StringComparison.InvariantCultureIgnoreCase))
                            .Select(newMember => new Member { 
                                                    Name = newMember.FirstName + " " + newMember.LastName,
                                                    eMail = newMember.Email,
                                                    IsVip = true
                                                });

Members = Members.Concat(newMembers);

The above is written without any IDE but I think you can figure out the solution to any syntax errors in it.

Answer 2

Some quick remarks:

• Bad naming: what is cd, am, m, mItem,...?
• Members should be camelCase.
• Do not do this a.eMail.ToLowerInvariant() == am.Email.ToLowerInvariant(), instead use String.Equals.
• Don't name your class Utilities, that's just asking for it to become an endless mess of various unrelated methods. I don't even see the point of the ObjectToStringOrEmpty method, since (string)mItem[companyCol] does the same thing and doesn't require me to go look in another class.
• If you're working with a List<T>, use Count instead of Count(). And use .Any() instead of .Count > 0.
• Can activeMembers ever be null?
• I'm worried by am.MembershipStatus == "Active" || am.MembershipStatus == "Pending"): these look like magic strings to me, and thus they should be const in a static class. Moreover, they look like they should be enum values.

---

You say "it's a large method of a live web services" and it shows. In these 30-odd lines it:

• gets data from a SharePoint server,
• converts these entries to Members,
• adds them to List<Member> Members,
• then retrieves more data from another source (MemberManager)
• and adds those to Members when appropriate.

These things should really be split up into smaller methods and possibly even be moved to separate classes.

If you move the Member creation inside foreach (SPListItem mItem in GetList(Url).Items) to a separate method e.g. ConvertToMember, you could replace the foreach with a LINQ chain:

var members = GetList(Url).Items.Select(ConvertToMember).ToList();

Same for the activeMembers logic, which could probably be reduced to something like this:

Members.AddRange(
    new MemberManager()
    .GetMoreMembers(Url + "/")
    .Where(am => (am.MembershipStatus == "Active" || am.MembershipStatus == "Pending")
        && CanBeAdded(am.Email)));

Answer 3

I suggest just a tiny little change as there's already been said a lot.

For the exact same purpose as your utility method:

Utilities.ObjectToStringOrEmpty(mItem[companyCol])

I very often use an extension like this one:

static class ObjectExtensions
{
    public static string ToStringOrEmpty(this object value)
    {
        return value == null ? string.Empty : value.ToString();
    }
}

this will make you code look simpler:

m.Company = mItem[companyCol].ToStringOrEmpty();

Answer 4

The following use IEnumerable Linq Expressions. Linq is about expression building. Most implementations you'll see use anonymous delegates defined locally; however, some expressions do not need to be defined within a function scope.

The PendingMember to Member Select Expression/Clause below, is one such expression (Note that PendingMember is the Mock Type I'm defining for the am type in your code snippet). It does not require/use an instance outside of it's anonymous function scope.

Mock Models

    public class SPListItem
    {
        public SPListItem()
        {
            Items = new System.Dynamic.ExpandoObject();
            Items.companyCol = "Anonymous Inc.";
            Items.emailCol = "[email protected]";
            Items.commentCol = String.Empty;
            Items.msCol = "Anonymous";
        }

        public int ID { get; set; }
        public string Title { get; set; }
        public string this[string index]
        {
            get { return (string)(Items as IDictionary<string, object>)[index]; }
            set { (Items as IDictionary<string,object>)[index] = value; }
        }


        private dynamic Items { get; set; }

    }

    public class Member
    {
        public int ID { get; set; }
        public string Name { get; set; }
        public string Company { get; set; }
        public string eMail { get; set; }
        public string Comment { get; set; }
        public string Membership { get; set; }
        public bool IsVip { get; set; }

    }

    public class PendingMember
    {
        public int ID { get; set; }
        public string Email { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string MembershipStatus { get; set; }
    }

    public static dynamic GetList(string Url)
    {
        dynamic result = new System.Dynamic.ExpandoObject();

        var list = new List<SPListItem>(new SPListItem[]
        {
            new SPListItem() {
                 ID = 0, Title = "Title 0"
            },
            new SPListItem() {
                 ID = 1, Title = "Title 1"
            },
            new SPListItem() {
                 ID = 2, Title = "Title 2"
            },
            new SPListItem() {
                 ID = 3, Title = "Title 3"
            },
        });

        list.First()["emailCol"] = "[email protected]";

        result.Items = list;
        return result;
    }

    public class MemberManager
    {
        public IEnumerable<PendingMember> GetMoreMembers(string url)
        {
            return new PendingMember[]
            {
                new PendingMember()
                {
                    FirstName = "John",
                    LastName = "Doe",
                    ID = 10,
                    Email = "[email protected]",
                    MembershipStatus = "pending"
                },
                new PendingMember()
                {
                    FirstName = "Jane",
                    LastName = "Doe",
                    ID = 1,
                    Email = "[email protected]",
                    MembershipStatus = "active"
                },
                new PendingMember()
                {
                    FirstName = "Jason",
                    LastName = "Bourne",
                    ID = 2,
                    Email = "[email protected]",
                    MembershipStatus = "pending"
                }
            }.AsEnumerable();
        }
    }

    public static class Utilities
    {
        public static string ObjectToStringOrEmpty(string value)
        {
            return (String.IsNullOrEmpty(value)) ? "" : value;
        }
    }

Program Entry Point

    readonly static Func<PendingMember, Member> SelectPendingMemberToMemberClause = (active) => new Member()
    {
        Name = active.FirstName + " " + active.LastName,
        eMail = active.Email,
        IsVip = true
    };


    static void Main(string[] args)
    {
        string Url = "";
        string companyCol = "companyCol",
            emailCol = "emailCol",
            commentCol = "commentCol",
            msCol = "msCol";

        // this Function Delegate uses 'companyCol', 'emailCol', 'commentCol', 'msCol'.. so we're setting it locally.
        Func<SPListItem, Member> SelectSPListItemToMemberClause = mItem => new Member()
        {
            ID = mItem.ID,
            Name = mItem.Title,
            Company = Utilities.ObjectToStringOrEmpty(mItem[companyCol]),
            eMail = Utilities.ObjectToStringOrEmpty(mItem[emailCol]),
            Comment = Utilities.ObjectToStringOrEmpty(mItem[commentCol]),
            Membership = Utilities.ObjectToStringOrEmpty(mItem[msCol])
        };

        // the `<T> as IEnumerable<SPListItem> `cast is here only because I'm using dynamic object for the mocking.. 
        List<Member> members = (GetList(Url).Items as IEnumerable<SPListItem>).Select(SelectSPListItemToMemberClause).ToList();



        // if (DateTime.Now < row.EndDate) {

        // This Function Delegate uses 'members', so we're setting it locally...
        Func<PendingMember, bool> WhereActiveOrPendingStatusExcludingMembersClause = (active) => (active.MembershipStatus.ToLower() == "active" || active.MembershipStatus.ToLower() == "pending") && !(members.Any((member) => member.eMail.ToLowerInvariant() == active.Email.ToLowerInvariant()));


        IEnumerable<PendingMember> cd = new MemberManager().GetMoreMembers(Url + "/");


        members.AddRange(cd.Where(WhereActiveOrPendingStatusExcludingMembersClause)
            .Select(SelectPendingMemberToMemberClause).ToArray());


        //}

        // members.ToArray();

        // The next part prints to console, and is outside of the scope of this answer...

        members.ForEach(member =>
        {
            string formatString = @"Name:= {0}, ID:= {1}, Email:= {2}, Company:= {3}, Membership:= {4}, IsVIP:= {5}, Comment:= ""{6}""";
            Console.WriteLine(String.Format(formatString, member.Name, member.ID, member.eMail, member.Company, member.Membership, member.IsVip, member.Comment));
        });

        Console.WriteLine(value: "Press [Enter] to Proceed");
        ConsoleKey key;
        do
        {
            key = Console.ReadKey().Key;
        } while (key != ConsoleKey.Enter);
    }

OUTPUT

Name:= Title 0, ID:= 0, Email:= [email protected], Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 1, ID:= 1, Email:= [email protected], Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 2, ID:= 2, Email:= [email protected], Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= Title 3, ID:= 3, Email:= [email protected], Company:= Anonymous Inc., Membership:= Anonymous, IsVIP:= False, Comment:= ""
Name:= John Doe, ID:= 0, Email:= [email protected], Company:= , Membership:= , IsVIP:= True, Comment:= ""
Name:= Jason Bourne, ID:= 0, Email:= [email protected], Company:= , Membership:= , IsVIP:= True, Comment:= ""
Press [Enter] to Proceed