HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Comparing two lists

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
twocomparinglists

Problem

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 Members = new List();
    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  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();

Solution

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

List Members = new List();
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 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.

Code Snippets

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);
}
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 + "/");
am.MembershipStatus == "Active" || am.MembershipStatus == "Pending"
if (activeMembers != null || activeMembers.Count() > 0)

Context

StackExchange Code Review Q#107523, answer score: 11

Revisions (0)

No revisions yet.