List<T>.Remove(T item)

[mp]

is there a good way to "index" custom objects in a list for removal?

similar to SortedDictionary.Remove(key)

how to do that with List<Criteria>.Remove( ? )

i found two ways so far, comments?
from http://stackoverflow.com/questions/2742313/list-remove-question
1)
....said to be using Link ?(of which i know exactly nothing)
Customer customer = collCustList.Where(c => c.Number == 99 && c.Type == "H"
/* etc */).FirstOrDefault();
if (customer != null)
{
collCustList.Remove(customer);
}

....or ....
2)
collCustList.RemoveAll(c => c.Number == 99 && c.Type == "H" /* etc */);


i'm using the .Where version and it seems to work.
//this fires when user checks an item in listview
private void lvCriteriaList_ItemCheck(object sender,
System.Windows.Forms.ItemCheckEventArgs e)
{
string key=this.lvCriteriaList.Items[e.Index].SubItems[1].Text;
string value=this.lvCriteriaList.Items[e.Index].Text;

if (_criteriaChoiceList == null)
{
_criteriaChoiceList = new List<Criteria>();
}

if (e.NewValue == CheckState.Checked)
{
AddCriteria(key, value);
}
else if (e.NewValue == CheckState.Unchecked)
{
RemoveCriteria(key, value);
}
DisplayCriteria();
}

private void AddCriteria(string key, string value )
{
_criteriaChoiceList.Add(new Criteria(key, value));
}
private void RemoveCriteria(string key, string value)
{
Criteria criteria1 = _criteriaChoiceList.Where(c => c.Key ==
key && c.Value == value ).FirstOrDefault();
if (criteria1 != null)
{
_criteriaChoiceList.Remove(criteria1);
}
}

thanks for any comments
mark

 

[Arne Vajhøj]

is there a good way to "index" custom objects in a list for removal?

similar to SortedDictionary.Remove(key)

how to do that with List<Criteria>.Remove( ? )

i found two ways so far, comments?
from http://stackoverflow.com/questions/2742313/list-remove-question
1)
...said to be using Link ?(of which i know exactly nothing)
Customer customer = collCustList.Where(c => c.Number == 99&& c.Type == "H"
/* etc */).FirstOrDefault();
if (customer != null)
{
collCustList.Remove(customer);
}

...or ....
2)
collCustList.RemoveAll(c => c.Number == 99&& c.Type == "H" /* etc */);

If Criteria implements Equals and GetHashCode, then a simple:

mylist.Remove(objtoremove);

should work.

Arne

 

[mp]

If Criteria implements Equals and GetHashCode, then a simple:

mylist.Remove(objtoremove);

should work.

Arne

how do you get a reference to objtoremove? create a new one with those props
and use it?

 

[mp]

If the Criteria type overrides Equals(), then the theory is that you can
create a new instance of Criteria, and the Remove() method will remove the
other instance that was already in the list that is equal to the new one
you created (according to the Equals() method).

Pete

seems a bit wasteful to create a new object in order to get rid of an
existing one
maybe not electronically, i'm just thinking in principle...theoretically...

no one commented on the .RemoveAll version either...seems similar to
creating
a new object to match...without having to actually create it???
any thoughts on that?
thanks
mark

 

[mp]

If Criteria implements Equals and GetHashCode, then a simple:

[]
If Equals() isn't overridden, then the following will work as well (i.e.
the LINQ version is overkill and less efficient):

int index = _criteriaChoiceList.Find(
c => c.Key == key && c.Value == value);

if (index >= 0)
{
_criteriaChoiceList.RemoveAt(index);
}
[]

Pete

..Find seems to want to return a Criteria object which won't cast to int???
oh, you meant .FindIndex?

 

[mp]

Technically, GetHashCode() doesn't even need to be implemented (but it
would be rare and incorrect to find a class that overrode Equals() but not
GetHashCode()).

If Equals() isn't overridden, then the following will work as well

i tried overriding as suggested

the .FindIndex option seems to work even with Equals overridden
but when i try to serialize now i get an error in the equals override
stack overflow
don't know why?
(if i comment out the overrides and implement IEquatable it works)

maybe i wrote something wrong?
[Serializable()]
public class Criteria : IEquatable<Criteria>
{
public string Key { get; set; }
public string Value { get; set; }

public Criteria()
{ }
public Criteria(string key, string value)
{
Key = key;
Value = value;
}

bool System.IEquatable<Criteria>.Equals(Criteria o)
{
if(o == null) return false;
return this.Key == o.Key &&
this.Value == o.Value ;
}

public override bool Equals(Object o)
{
return o != null &&
this.GetType() == o.GetType() &&
this.Equals ( (Criteria)o );
}

public override int GetHashCode()
{
return Key.GetHashCode() ^
Value.GetHashCode();
}
}

....the serialize code...
public static void StoreCriteriaChoices(List<Criteria> criteriaChoices,
string fileName)
{
XmlSerializer serializer=null ;
try
{
using (FileStream fStream = new FileStream(fileName,
FileMode.Create))
{
try
{
serializer = new XmlSerializer(typeof(List<Criteria>));
serializer.Serialize(fStream, criteriaChoices);
}
catch (Exception ex)
{
Debug.Print("Unable to StoreCriteriaChoices...Problem with
XmlSerializer ");
Debug.Print(ex.Message);
throw;
}
}
}
catch (Exception ex)
{
Debug.Print("Unable to StoreCriteriaChoices...Problem with
FileStream <" + fileName + ">");
Debug.Print(ex.Message);
throw;
}
}

what am i doing wrong?
thanks
mark

 

[mp]

Yeah, sorry.typo. Made the exact same one in my other reply (guess where
I got the code for that post ). Sorry about that!

you're so unreliable

just kidding

 

[mp]

Well, the LINQ approach creates more than just one more object. You get a
whole new IEnumerable<T> object, and _then_ from that a new IEnumerator<T>
object. Not to mention the delegate instance for the predicate passed to
the Where() method (same is true for any of the predicate-based versions
though).

The only way to avoid creating _some_ new object is to write the loop
explicitly. Which you can do. But it's not as pretty as the other
approaches.


Yeah. I didn't bother commenting, because it didn't seem like you were
inclined to follow that example anyway. But IMHO it's about the worst
option: first, "RemoveAll" implies to the reader that there may be more
than one, which is misleading (in this case); second, the method can't
short-circuit once it finds and removes the item of interest.it will
always scan the entire list.

It's a bit shorter than any of the other approaches, but not in a way that
improves the code. If you really wanted a shorter way to remove an item
from a list, you'd do something like this extension method:

public static bool Remove<T>(this List<T> list, Predicate<T> predicate)
{
int index = list.Find(predicate);

if (index >= 0)
{
list.RemoveAt(index);
return true;
}

return false;
}

Then you can write:

_criteriaChoiceList.Remove(c => c.Key == key && c.Value == value);

.which is a few characters shorter than the RemoveAll() version, can
short-circuit, and is named more appropriately.

Pete

that looks very nice
would you lean toward that as the 'best' choice?
does that still require overridding Equals and GetHashCode?
(i'll give it a try)
as i mentioned in another post,
Overridding Equals makes serialization not work anymore

 

[mp]

On 1/19/11 9:06 PM, mp wrote: []

Then you can write:

_criteriaChoiceList.Remove(c => c.Key == key && c.Value == value);

.which is a few characters shorter than the RemoveAll() version, can
short-circuit, and is named more appropriately.

Pete

the .RemoveAt( .FindIndex(...) style works and is fine with me

the above method
_criteriaChoiceList.Remove(c => c.Key == key && c.Value == value);

also works if i create a static class to put the .Remove definition in
(it didn't work when i put it in the class that 's doing the removing)

and i had to eliminate the overridding of equals to get serializaation back
working
without raising a stackoverflow error inside the Equals method

thanks
mark

 

[mp]

[...]
what am i doing wrong?

Two things:

. Your Equals() method is recursive
. Your hash code calculation is poor
[]

thanks for the explanation
i got that code from
http://stackoverflow.com/questions/2742313/list-remove-question

maybe there's a reason the site is called stack overflow?

Pete

lacking an imperative to use that method i'm going with .FindIndex
thanks
mark

 

[Arne Vajhøj]

how do you get a reference to objtoremove? create a new one with those props
and use it?

Yes.

If it implements Equals and GetHashCode.

Arne

 

[Arne Vajhøj]

seems a bit wasteful to create a new object in order to get rid of an
existing one
maybe not electronically, i'm just thinking in principle...theoretically...

It is as efficient as removing from a List<> can be.

And at least to me it seems to match the natural way
of thinking.

no one commented on the .RemoveAll version either...seems similar to
creating
a new object to match...without having to actually create it???
any thoughts on that?

Use RemoveAll if you need to remove more than one object
from the list.

But assuming you do not have duplicates in the list, then
that will require testing for something else than logical
equals.

Arne

 

[Arne Vajhøj]

Technically, GetHashCode() doesn't even need to be implemented (but it
would be rare and incorrect to find a class that overrode Equals() but
not GetHashCode()).

Those two are Siamese twins.

If Equals() isn't overridden, then the following will work as well (i.e.
the LINQ version is overkill and less efficient):

int index = _criteriaChoiceList.Find(
c => c.Key == key && c.Value == value);

if (index >= 0)
{
_criteriaChoiceList.RemoveAt(index);
}

That seems as a lot of code to me.

Arne

 

[Arne Vajhøj]

that looks very nice
would you lean toward that as the 'best' choice?
does that still require overridding Equals and GetHashCode?
(i'll give it a try)
as i mentioned in another post,
Overridding Equals makes serialization not work anymore

That is due to a bug in the code.

Serialization should work fine for classes implementing
Equals and GetHashCode.

I would even claim that in most cases where classes
are serialized then Equals and GetHashCode should
implement those.

If you serialize an object to a file and then
deserialize from the file again, then the new
object should be equal to the old object.

That is not the case with the object identity inherited
from object.

Arne

 

[Arne Vajhøj]

[...]
That seems as a lot of code to me.

That seems like a pointless comment to me.

Then you have missed the point.

Seriously, you're not going to accomplish the same result with less
code. There were only three statements in the code you're objecting. It
takes a lot more than that to override Equals() and GetHashCode() in
every class where you want to be able to remove instances,

But adding those 8 lines of code:
* place the code where it belongs with the class itself
* is reusable all over the app

Those 3 lines of code is placed in code that contains
the logic, which should be as readable as possible, not cluttered
with various utility stuff. And if the same logic is needed
100 places in the logic, then it is no longer 3 lines of code
but 300 lines of code, because the approach is not reusable.

and the LINQ
version is just plain dumb.

I completely agree with that.

If you really care about the extra lines of code, it's trivial to turn
the three statements you don't like into an extension method that you
can use anywhere you would have used the LINQ version.

That would make it reusable, so that would fix one of the
problems (and I would say the most important problem).

Arne

 

[Arne Vajhøj]

[...]

Seriously, you're not going to accomplish the same result with less
code. There were only three statements in the code you're objecting. It
takes a lot more than that to override Equals() and GetHashCode() in
every class where you want to be able to remove instances,

But adding those 8 lines of code:
* place the code where it belongs with the class itself
* is reusable all over the app

But what if the definition of "equals" differs according to use? What if
normally, it does not make sense for the objects to be comparable at
all, and so that use case should not be supported generally? Or even if
it does make sense, if the object is relatively expensive to
instantiate, or creating an instance of the object requires setting a
bunch of data that isn't relevant to the search condition?

There can be situations where Equals and GetHashCode is not suitable.

But a Criteria class as used by OP does not seem likely to fell
in this category.

More generally if something is to be stored in collections Equals
and GetHashCode should almost always be there or it will eventually
result in problems with unexpected behavior.

Long running constructors is not a good design and should be avoided.

IMHO, implementing Equals() and GetHashCode() is something one does if
the object _generally_ needs to be comparable with other objects of the
same type. It's not a way to solve some problem (i.e. removing a
specific object from a list) that is otherwise orthogonal to the type's
design.

It can happen.

But of one need to remove based on a non natural comparison then
RemoveAll with predicate would be more obvious, because for a
non natural comparison the risk of more than one match seems
very real.

[...]

If you really care about the extra lines of code, it's trivial to turn
the three statements you don't like into an extension method that you
can use anywhere you would have used the LINQ version.

That would make it reusable, so that would fix one of the
problems (and I would say the most important problem).

Hopefully you did notice my other reply to Mark in which I posted
exactly that approach. It's important to be able to distinguish between
code posted in a newsgroup to demonstrate a technique, and what someone
would do in a real program to make use of that code (i.e. not necessary
just copy and paste the code everywhere they want to do something like
that ).
True.

Still, you say "the most important problem". What other problems do you
see?

The major problem was the reusability. The other problem was
where the logic belongs.

Arne

 

[Arne Vajhøj]

[...]
But of one need to remove based on a non natural comparison then
RemoveAll with predicate would be more obvious, because for a
non natural comparison the risk of more than one match seems
very real.

Sorry, can't agree there. There are lots of good reasons to remove an
object based on a "non-natural comparison" and yet know that only one
such object exists.

It can happen.

But I would not write special code without knowing it to be the case.

RemoveAll() is great when you know there could be more than instance.
It's a waste of time otherwise.

Searching to the end after finding one cost a little, but I am
not so concerned about that.

I would not add a single line of code to avoid it unless
indications that it could be a problem.

[...]

Still, you say "the most important problem". What other problems do you
see?

The major problem was the reusability. The other problem was
where the logic belongs.

That's entirely scenario-dependent. Quite often, the logic cannot exist
in the type being stored in the list.

It happen.

But as a starting point I would expect it to belong with the type.

There's no reason to criticize a
three-statement snippet of code on that basis.

Why not.

I do not think those 3 statements was an optimal solution.

Arne

 

[Arne Vajhøj]

[...]

Sorry, can't agree there. There are lots of good reasons to remove an
object based on a "non-natural comparison" and yet know that only one
such object exists.

It can happen.

But I would not write special code without knowing it to be the case.

No one's asked you to. In any case, .NET is full of "special code" that
you never use, even in the libraries your programs actually load into
memory. It's ridiculous to object to a three-line method that can be
used anywhere on the basis of it being "special code".

Why.

I would always argue for clean solution unless it is deemed
absolutely necessary to do otherwise.

No reason to learn anybody bad habits.

It's not your code, and not your decision whether to be concerned or not.

Well - this is usenet.

People ask questions and get answer to what they ask for and
occasionally answers to what they don't ask for.

Being concerned about one O(n) having higher constant than
another O(n) is one of those things that you should not be
concerned with until it is proven to be a problem.

That's your prerogative.

My own opinion is that calling RemoveAll() is not only inefficient, it
carries a false implication regarding both the nature of the contents of
the list (the reader has to stop and wonder, can there be multiple
elements that actually meet this criteria?) and the nature of the intent
of the code (even if the list today is guaranteed to have only a single
element meeting that criteria, if and when it changes, does the code
then need to change, or at that section of code was it actually
desirable to remove all of them?).

I would expect so.

A couple of extra lines of code is a completely inconsequential price to
pay in order to gain both efficiency _and_ semantic precision at the
same time.

Well - lines of code is what cost money.

And I find the semantic precision very questionable.

[...]
That's entirely scenario-dependent. Quite often, the logic cannot exist
in the type being stored in the list.

It happen.

But as a starting point I would expect it to belong with the type.

What you expect is not the point.

The point is that it is good OOP to put functionality for a class
in the class.

The point here is to offer Mark the various options that _could_ work
for him. Debating which one is best is pointless; only he can make that
determination, and the answer will necessarily be different for
different scenarios.

I believe in that there are principles in good software engineering
not that it depends on scenarios.

Putting functionality for a class in the class itself and minimizing the
code in the business logic are good principles.

Unless there are very specific reasons to do otherwise they should
be followed.

It is not all is equally good and it depends.

Even if adding equality features to the type makes the most sense today,
he may run into a situation in the future where it doesn't. Having
offered him the bigger picture, he (and anyone else reading this thread)
will have a more complete set of tools to work with.

I completely agree with that.

But since you think it is good for the OP to get a bigger picture,
then why did you object to me bring the topic of code size on the
table??

It depends on what you're trying to optimize. _Maybe_ if I had said that
the code was _the_ most optimal solution for anyone's needs, your reply
might have made a little sense.

As it was, it was simply needlessly critical, adding nothing to the
discussion except negativity.

I think it brought a lot of good software engineering principles
on the table.

That is not so bad.

Arne

 

[Arne Vajhøj]

[...]
That's entirely scenario-dependent. Quite often, the logic cannot
exist
in the type being stored in the list.

It happen.

But as a starting point I would expect it to belong with the type.

What you expect is not the point.

The point is that it is good OOP to put functionality for a class
in the class.

That's a tautological argument. It works only if you start by assuming
that functionality belongs in the class, which is what you're trying to
prove in the first place.

It is not.

The logic is:

putting functionality for class in the class itself is good OOP => this
comparison should be put in this class

There is not any assuming what is to be proven in that.

[...]

Even if adding equality features to the type makes the most sense today,
he may run into a situation in the future where it doesn't. Having
offered him the bigger picture, he (and anyone else reading this thread)
will have a more complete set of tools to work with.

I completely agree with that.

But since you think it is good for the OP to get a bigger picture,
then why did you object to me bring the topic of code size on the
table??

I objected to the unwarranted, _unqualified_ criticism.

Well - my first post actually clearly indicated what I saw as
the main problem.

Si it is not unqualified.

Pointing out specific concerns, raising whatever topics you feel are
relevant as much as you want, those are all sensible ways to have a
discussing. That's not what you did.

It is exactly what I did.

The fact is, the code I posted is not inherently bad, as your response
implied. If you would like to point out scenarios in which there are
better ways to approach the problem, that's not offensive at all. But
this black-and-white implication that there's something fundamentally
wrong with my suggestion is.

Well - my main point is that the alternative should be used by
default unless there are specific reasons to do otherwise.

Such reasons can exist. I am not denying that.

[...]

As it was, it was simply needlessly critical, adding nothing to the
discussion except negativity.

I think it brought a lot of good software engineering principles
on the table.

That is not so bad.

No, the end result is positive. But that's no reason to consider the
means appropriate. We can have good technical discussions without
casting things as either/or, and without making blanket criticisms.

I don't think it was a blanket criticism.

Arne

 

[Arne Vajhøj]

[...]
That's entirely scenario-dependent. Quite often, the logic cannot
exist
in the type being stored in the list.

It happen.

But as a starting point I would expect it to belong with the type.

What you expect is not the point.

The point is that it is good OOP to put functionality for a class
in the class.

That's a tautological argument. It works only if you start by assuming
that functionality belongs in the class, which is what you're trying to
prove in the first place.

It is not.

The logic is:

putting functionality for class in the class itself is good OOP => this
comparison should be put in this class

There is not any assuming what is to be proven in that.

[...]
Even if adding equality features to the type makes the most sense
today,
he may run into a situation in the future where it doesn't. Having
offered him the bigger picture, he (and anyone else reading this
thread)
will have a more complete set of tools to work with.

I completely agree with that.

But since you think it is good for the OP to get a bigger picture,
then why did you object to me bring the topic of code size on the
table??

I objected to the unwarranted, _unqualified_ criticism.

Well - my first post actually clearly indicated what I saw as
the main problem.

Si it is not unqualified.

Pointing out specific concerns, raising whatever topics you feel are
relevant as much as you want, those are all sensible ways to have a
discussing. That's not what you did.

It is exactly what I did.

The fact is, the code I posted is not inherently bad, as your response
implied. If you would like to point out scenarios in which there are
better ways to approach the problem, that's not offensive at all. But
this black-and-white implication that there's something fundamentally
wrong with my suggestion is.

Well - my main point is that the alternative should be used by
default unless there are specific reasons to do otherwise.

Such reasons can exist. I am not denying that.

[...]
As it was, it was simply needlessly critical, adding nothing to the
discussion except negativity.

I think it brought a lot of good software engineering principles
on the table.

That is not so bad.

No, the end result is positive. But that's no reason to consider the
means appropriate. We can have good technical discussions without
casting things as either/or, and without making blanket criticisms.

I don't think it was a blanket criticism.

But before I start to sound too aggressive then maybe I should
clarify that even though I did not like the solution in
this context then:
* I do not think it was a bad solution
* I think that it was not the best solution in the most
common scenarios
* I completely agree that in some scenarios it will be
the best solution

Arne