Remove object from generic list by id

Question

I have a domain class like this:

public class DomainClass
{
  public virtual string name{get;set;}
  public virtual IList<Note> Notes{get;set;}
}

How would I go about removing an item from the IList<Note>? I would be able to do it if it was a List but it has to be an IList as I am using Nhibernate for my persistance layer.

Ideally I wanted a method like this in my domain class:

public virtual void RemoveNote(int id)
{
   //remove the note from the list here

   List<Note> notes = (List<Note>)Notes

   notes.RemoveAll(delegate (Note note)
   {
       return (note.Id = id)
   });
}

But I can't cast the IList as a List. Is there a more elegant way round this?

Anglofrench

Interesting problem, you don't want to cast to the concrete type because you don't know it before. I guess if you loop on all elements would work but would be slow. I don't know the answer, I wonder if selecting the node to delete with a LINQ query would help then every concrete class used at runtime will execute the linq query faster or slower depending on the list type, sorted, ordered or not... – Haug

Do you have notes with the same ID? If not you might want to use a IDictionary<int, Note> instead – Cima

@Cima no the ID's will be unique – Anglofrench

Answer 1

You could filter out the items you don't want and create a new list with only the items you do want:

public virtual void RemoveNote(int id)
{
   //remove the note from the list here

   Notes = Notes.Where(note => note.Id != id).ToList();
}

Lecturer

Nice - i was going at it from completely a different angle. How efficient will this be though? – Anglofrench

There shouldn't be much of a difference. You're having to traverse the whole list anyway. – Baryon

Answer 2

Edit2: This method doesn't require casting to a List!

foreach (var n in Notes.Where(note => note.Id == id).ToArray()) Notes.Remove(n);

or...

Notes.Remove(Notes.Where(note => note.Id == id).First());

The first one is the best.
The second one will throw an exception if no notes have that id.

Edit: Thanks to Magnus and rsbarro for showing my mistake.

Wrand

Removing items from a list while iterating it, not gonna work. Do a .ToList() – Cima

@Cima is right, the first one will throw a InvalidOperationException: Collection was modified; enumeration operation may not execute.. – Presidentship

This could be simplified by writing: Notes.Remove(Notes.First(note => note.Id == id)); – Bumboat

Answer 3

You can either code it manually. The naive implementation is O(n*k) with n the number of items in the list, and k the number of items you want to remove. If you want to just remove a single item it is fast.

But if you want to remove many items then the native implementation becomes O(n^2) for many IList<T> implementations(including List<T>, no idea how NHibernate's list behaves) and you need to write a bit more code to get a O(n) RemoveAll implementation.

One possible implementation from an old answer: List, not lose the reference

The trick with this implementation is that in moves the kept items to the beginning of the list in O(n). Then it keeps removing the last item of the list(which is usually O(1) since no elements need to move), so the truncation becomes O(n) total. This means the whole algorithm is O(n).

Diantha

Answer 4

If you can change the datastructure I would suggest using a Dictionary. Than you can go with:

public class DomainClass
{
  public virtual string name{get;set;}
  public virtual IDictionary<int, Note> Notes {get; set;}

  //Helper property to get the notes in the dictionary
  public IEnumerable<Note> AllNotes
  {
    get
    {
      return notes.Select (n => n.Value);
    }
  }

  public virtual void RemoveNote(int id)
  {
     Notes.Remove(id);
  }

}

If ID is not unique use IDictionary<int, IList<Note>> instead.

Cima

Answer 5

Please consider, that in some cases better to avoid public virtuals, use template method pattern in such a way:

 public void Load(IExecutionContext context) 
 { 
      // Can safely set properties, call methods, add events, etc...
      this.Load(context);            
      // Can safely set properties, call methods, add events, etc. 
 }

 protected virtual void Load(IExecutionContext context) 
 {
 }

Minimal

I'd like to see some kind of justification for avoiding public virtuals. – Hap

@spender, by overriding a virtual method you can corrupt base class behaviour by skipping a base.Method() call. This approach also well known as NVI idiom. – Minimal

@Minimal Calling the base method in an overridden method may or may not be appropriate depending on what the base method does e.g. if you overrode ToString(), in most cases there would be no value to calling base.ToString() in your overridden method. – Traceable

@Ben Robinson, I must agree, you are exactly right. I should midify my post by adding "in some cases consider". Thanks for a point! – Minimal

@Ben Robinson, regarding overriding of the ToString(), lets image PersonBase base class with properties Id and Name, so its ToString() will return "Id=.. Name...", and all nested classes should just concatenate base.ToString() + own ToString() implementation – Minimal

Answer 6

You can receive an array of items for removing. Than remove them from list in loop. Look at this sample:

IList<int> list = new List<int> { 1, 2, 3, 4, 5, 1, 3, 5 };

var valuesToRemove = list.Where(i => i == 1).ToArray();

foreach (var item in valuesToRemove)
{
    list.Remove(item);
}

Lemaster

Answer 7

Just List.RemoveAt("ID");

or better..

try
{
     List.RemoveAt("ID");
}
catch
{
     Console.WriteLine("Out of bounds.");
}

Lifeline

Answer 8

My solution, first make a Hashset list for the items to be deleted, then call RemoveAll items which contains the id of the HashSet:

HashSet<Guid> vListIdToDelete = new HashSet<Guid>();//TODO fill here
vOtherList.RemoveAll(p => vListIdToDelete.Contains(p.ID));

Kiser