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();

• IEatBagels: Is this a method? If so, please include the method's signature (the parameters and all)
• Mathematics: @TopinFrassi it's a large method of a live web services and I am not allowed to touch anything else to be honest.
• IEatBagels: I think the question as-is is fine. I just thought it'd be easier to review with the full method but that's fine
• Mathieu Guindon: I'd also mention that if your method is any longer than that, it's very likely doing too many things. One shouldn't be afraid to refactor poorly written code.
• Mathematics: @Mat'sMug you are right, services were designed very poorly and because they are already rolled out it's difficult for me to refactor them, but yes it definitely should be broken down into several methods, totally agree.

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.

• Mathematics: Your answer is complete but now requirements are bit changed, I need to update member's VIP property to true, if "!Members.any(..." false, i may ask another question later on

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)));

• Mathematics: Thanks for useful comments, But "(string)mItem[companyCol] does the same thing" it doesn't, (string)object returns null not empty
• BCdotWEB: @PleaseTeach True, but I'd argue that returning string.Empty would be a bad thing, and that the code that can't deal with a null string should be revised.
• Mathematics: I agree, working on ex developers code, will see if I could improve it
• Mathematics: why .Any() is better then Count()
• BCdotWEB: @PleaseTeach stackoverflow.com/a/305156/648075

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

• Jeroen Vannevel: What.. happened here? Why did the codebase multiply tenfold? What necessitated the introduction of dynamic (which is one of the most expensive operations you can do)?
• Brett Caswell: @JeroenVannevel, the answer to both questions should be obvious.. the code base includes mock types so it can be run, and dyanmic is a used to mock the SPListItem method which is apparently (and likely) used to retrieve info from a web service (which is a runtime consideration that uses IO and deserialization)..
• RubberDuck: It's not obvious. It reads like a code dump and I can't imagine this being very useful to OP.