11
\$\begingroup\$

I want to use a better syntax than nested foreach statements to overwrite the initial list with items from the second list.

In the code below:

  • I want to overwrite initialList with those in secondList that have the same Value (Remove Red).
  • Use the items in secondList where Value was the same (Yellow)

New initialList list should include (Green and Yellow).

static void Main(string[] args)
{
    int useProd = 2;
    int useDomain = 0;

    var person1 = new Person() { prodId = 1, Value = "foo", domainId = 0, Name = "Red" };
    var person2 = new Person() { prodId = 1, Value = "bar", domainId = 0, Name = "Green" };
    var person3 = new Person() { prodId = 1, Value = "foo", domainId = 1, Name = "Yellow" };

    var initialList = new List<Person>();
    initialList.Add(person1);
    initialList.Add(person2);

    var secondList = new List<Person>();
    secondList.Add(person3);

    List<Person> personsToRemove = new List<Person>();
    List<Person> personsToUpdate = new List<Person>();

    foreach (var pers1 in initialList)
    {
        foreach (var pers2 in secondList)
        {
            if (pers1.Value == pers2.Value)
            {
                personsToRemove.Add(pers1);
                personsToUpdate.Add(pers2);
            }
        }
    }
    foreach (var remPers in personsToRemove)
    {
        initialList.Remove(remPers);
    }
    foreach (var updPers in personsToUpdate)
    {
        initialList.Add(updPers);
    }
    foreach (var item in initialList)
    {
        Console.WriteLine(String.Format("Value: {0}, prodId: {1}, domainId: {2}, Name: {3}", item.Value, item.prodId, item.domainId, item.Name));
    }

    Console.ReadKey();
}
public class Person
{
    public int prodId { get; set; }
    public string Value { get; set; }
    public int domainId { get; set; }
    public string Name { get; set; }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. I hope you get some good answers. \$\endgroup\$ – Heslacher Feb 10 '15 at 16:35
  • \$\begingroup\$ It's unclear to me what is being done exactly. Can you clarify what the business idea behind the code is? \$\endgroup\$ – Jeroen Vannevel Feb 10 '15 at 16:40
  • \$\begingroup\$ @JeroenVannevel The code is overly simplified from business code. I need to Union two lists. I cannot just add both together, I need to overwrite the initialList objects with the objects from secondList where property Value is the same. And I also need all object from secondList that dont exists in initialList added. When foreach statements finds initial.Value == second.Value it Remove(initial object), and Add(second object) instead. Is there another way to do this in Linq or Lambda? \$\endgroup\$ – Niike2 Feb 10 '15 at 17:01
  • \$\begingroup\$ When you say "I also need all object from secondList that dont exists in initialList", how do you determine something doesn't exist in initialList? What determines uniqueness? \$\endgroup\$ – Jeroen Vannevel Feb 10 '15 at 17:04
  • \$\begingroup\$ The uniqueness comes from other properties like Id column in the DB, as I said its a very simplified example, theres more properties on the object. When I create initialList and secondList in the business code they get populated from DB but secondList is differentiated on a Id column. \$\endgroup\$ – Niike2 Feb 10 '15 at 17:15
11
\$\begingroup\$

By using an IEqualityComparer<Person> which only compares the Value property like

public class PersonValueComparer : IEqualityComparer<Person>
{
    public bool Equals(Person x, Person y)
    {
        if (x == null && y == null) { return true; }
        if (x == null || y == null) { return false; }

        return x.Value == y.Value;
    }

    public int GetHashCode(Person obj)
    {
        if (obj == null || obj.Value == null) {return 0;}
        return obj.Value.GetHashCode();
    }
}  

you can by using linq methods replace the loops by

        PersonValueComparer valueComparer = new PersonValueComparer();
        initialList = secondList.Where(p => initialList.Contains(p, valueComparer))
            .Concat(initialList.Where(p => !secondList.Contains(p, valueComparer))).ToList();

We are first using the Where() method to filter the secondList for items which are in the initialList . Then we use Concat() to add the Person''s which are not in thesecondList`.


Based on the naming guidelines properties should be named using PascalCase casing.


You should always use meaningful and descriptive names for naming methods, variables, properties and classes so Sam the Maintainer sees at first glance what it is about.


Shortening variable names doesn't add any value but instead reduces readability which Sam the Maintainer doesn't like.


\$\endgroup\$
  • 2
    \$\begingroup\$ Isn't the .Select completely worthless in this example? \$\endgroup\$ – Bryan Boettcher Feb 10 '15 at 19:24
  • \$\begingroup\$ @insta you are right. Updated answer. \$\endgroup\$ – Heslacher Feb 10 '15 at 19:29
11
\$\begingroup\$

Since you mentioned in the comments that you want the union of the two lists, you could perform a LINQ Union. There are a few ways to do it:

Use IEnumerable<T>.Union(IEnumerable<T>)

If you go with the simpler overload for Union, you need to implement IEquatable<T> and override GetHashCode on Person.
For simplicity, my example will look only at the Value property:

public class Person : IEquatable<Person>
{
    public int prodId { get; set; }
    public string Value { get; set; }
    public int domainId { get; set; }
    public string Name { get; set; }
    public override bool Equals(object obj)
    {
       return Equals(obj as Person);
    }

    public override int GetHashCode()
    {
       return Value == null ? 0 : Value.GetHashCode();
    }

    public bool Equals(Person other)
    {
       return other != null
              && other.Value == Value;
    }
}

Example usage:

static void Main(string[] args)
{
    var list1 = new List<Person>
                {
                   new Person {Value = "bob"},
                   new Person {Value = "alice"},
                };

    var list2 = new List<Person>
                {
                   new Person {Value = "bob"},
                   new Person {Value = "charles"},
                };

    var unionedList = list1.Union(list2).ToList();
}

Use IEnumerable<T>.Union(IEnumerable<T>, IEqualityComparer<T>)

If you go with this option, you could use @Heslacher's equality comparer as the parameter for the Union call.

static void Main(string[] args)
{
    var list1 = new List<Person>
                {
                   new Person {Value = "bob"},
                   new Person {Value = "alice"},
                };

    var list2 = new List<Person>
                {
                   new Person {Value = "bob"},
                   new Person {Value = "charles"},
                };

    var unionedList = list1.Union(list2, new PersonValueComaparer()).ToList();
}

Order Matters

Since you want secondList to win in any conflicts, you would use secondList.Union(initialList), so any matches would defer to the objects in secondList.

\$\endgroup\$
  • \$\begingroup\$ @Heslacher I mentioned that at the end, but with the formatting, I can see that it could easily be missed. I will re-format the answer a little. \$\endgroup\$ – Dan Lyons Feb 11 '15 at 18:51
8
\$\begingroup\$

How about this:

var peopleToAdd = secondList.Where(p1 => initialList.Any(p2 => p1.Value == p2.Value)).ToList();
initialList.RemoveAll(p1 => secondList.Any(p2 => p1.Value == p2.Value));
initialList.AddRange(peopleToAdd);

Step 1 - look at the second list and find all people that need to be added to the first list. We are looking for all the people that have the same Value in both lists. Note: Execute .ToList() to get the result before the Step 2 is executed.

var peopleToAdd = secondList.Where(p1 => initialList.Any(p2 => p1.Value == p2.Value)).ToList();

Step 2 - Remove all the people from the first list that occur in the second list with the same Value.

initialList.RemoveAll(p1 => secondList.Any(p2 => p1.Value == p2.Value));

Note: Micro-optimization for the Step 2, you can use peopleToAdd instead of secondList since we've already discovered matching people.

initialList.RemoveAll(p1 => peopleToAdd.Any(p2 => p1.Value == p2.Value));

Step 3 - add all the people found in Step 1 to the initialList.

initialList.AddRange(peopleToAdd);
\$\endgroup\$
  • 1
    \$\begingroup\$ does my explanation make sense now? \$\endgroup\$ – Alexey Adamsky Feb 10 '15 at 19:18
  • 1
    \$\begingroup\$ Much Better. thank you. we like more explanation here on Code Review. thank you for taking the time to explain what is going on. \$\endgroup\$ – Malachi Feb 10 '15 at 19:40
5
\$\begingroup\$

What you are actually doing is updating the initialList with objects from secondList based on matching Value. The optimal solution would have an O(N+M) comparisons (N and M - number of elements in both lists, while your solution performs O(N*M) comparisons.

In order to make your code simpler I suggest to use the lookup table like this:

var lookup = secondList.ToLookup(person => person.Value);

initialList = initialList
    .Select(person => lookup[person.Value].FirstOrDefault() ?? person).ToList();

We "walk" through the secondList collection once to build a lookup table, and go through the initialList list once to replace elements that match the item in lookup.

\$\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.