Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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; }
}
share|improve this question
1  
Welcome to Code Review. I hope you get some good answers. –  Heslacher 17 hours ago
    
It's unclear to me what is being done exactly. Can you clarify what the business idea behind the code is? –  Jeroen Vannevel 17 hours ago
    
@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? –  Niike2 17 hours ago
    
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? –  Jeroen Vannevel 16 hours ago
    
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. –  Niike2 16 hours ago

4 Answers 4

Since you mentioned in the comments that you want the union of the two lists, you could perform a LINQ Union.

One option is to implement IEquatable<Person> 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();
}

Alternatively, you could use @Heslacher's equality comparer and the Union overload which takes in the comparer.

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

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

share|improve this answer
    
Unfortunately this doesn't produce the requested result. Using the provided lists of the op and calling initialList.Union(secondList).ToList(); will not result in Yellow beeing in the list. By switching the lists like secondList.Union(initialList).ToList(); it will succeed. –  Heslacher 2 hours ago

How about this:

var peopleToAdd = secondList.Where(p1 => initialList.Any(p2 => p1.Value == p2.Value));
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.

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

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);
share|improve this answer
1  
does my explanation make sense now? –  Alexey Adamsky 14 hours ago
1  
Much Better. thank you. we like more explanation here on Code Review. thank you for taking the time to explain what is going on. –  Malachi 14 hours ago
1  
This does not work as expected. Problem here is that the linq query which filters Person's to add will be executed after the call to RemoveAll so after this call the initialList doesn't contain any element which fit for the Where. By calling ToList() on secondList.Where(p1 => initialList.Any(p2 => p1.Value == p2.Value)) it will succeed. –  Heslacher 50 mins ago

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.


share|improve this answer
1  
Isn't the .Select completely worthless in this example? –  insta 14 hours ago
    
@insta you are right. Updated answer. –  Heslacher 14 hours ago

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.