tags:

views:

290

answers:

4

Does List.Contains(mystring) do a reference comparison or a value comparison? Eg I have this code:

/// <summary>
/// If the value isn't null or an empty string, 
/// and doesn't exist in the list, it adds it to the list
/// </summary>
static void AddToListIfNotEmpty(List<string> thelist, SqlString val)
{
  string value = val.ToString().Trim();
  if (!string.IsNullOrEmpty(value))
  {
    bool found = false;
    foreach (string x in thelist) if (x == value) found = true;
    if (!found) thelist.Add(value);
  }
}

Can i simplify the foreach and following line to:

if (!thelist.Contains(value)) thelist.Add(value);

Thanks

+4  A: 

Comparing for all strings is overloaded to be by value. Sorry didn't notice you had a SqlString, why not use it's ToString() method and add that? Your shortened method won't compile.

Yuriy Faktorovich
What do you mean it won't compile? It will compile just fine. `value` is `val` converted to a `string`.
Thorarin
My heat stroke is killing me still, you are correct.
Yuriy Faktorovich
+2  A: 

IList<T> uses Comparator<T>.Default to do the comparisons, and in turn that compares by value for String objects.

If you wanted to, you could move your items to an IDictionary<T, bool> or something similar, where you can specify your IComparator - specifying one that checks for reference. (even though in that case, you're better off with the foreach loop)

If you can use LINQ, you could create a statement there with a reference comparison, too.

Aviad Ben Dov
Comparing by reference makes no sense. There is no way a `SqlString` reference will be equal to that of a `string` already in the list?
Thorarin
You're correct, I didn't notice that. That might make the Equals method return false, too.In that case, using the IDictionary trick or LINQ is his option,
Aviad Ben Dov
Wait, I was confused a bit. He's comparing the ToString().Trim() value, not the SqlString itself. The Equals might work, and even the reference comparison if the strings are interned.
Aviad Ben Dov
Strings are not interned automatically, but the `String` class' equality operator compares by value, not by reference. To compare references, you'd have to cast it to `object` first.
Thorarin
Casting will obviously not help. That's why I suggested LINQ or `IDictionary`.
Aviad Ben Dov
I think i've confused things by having my sqlstring in there. You can assume that everything is strings in my example.
Chris
I know. Still, my answer stands. :)
Aviad Ben Dov
+1  A: 

From MSDN of List<T>.Contains

This method determines equality using the default equality comparer EqualityComparer<T>.Default for T, the type of values in the list.

... The Default property checks whether type T implements the IEquatable<T> generic interface and if so returns an EqualityComparer<T> that uses that implementation. Otherwise it returns an EqualityComparer<T> that uses the overrides of Object.Equals and Object.GetHashCode provided by T.

Looking at reflector (and by the principle of least surprise), String Equality has value type semantics - so they are equal if they have the same strings. Both Equals(Object) and IEquatable.Equals(T) delegate to String::EqualsHelper

Gishu
+1  A: 

Yes, you can simplify (in terms of code, not complexity*) by removing the foreach, but your simplification is not functionally identical to your original.

The simplied, functional equivalent would be:

static void AddToListIfNotEmpty(List<string> thelist, SqlString val)
{
    string value = val.ToString().Trim();
    if (value != string.Empty && !thelist.Contains(value))
        thelist.Add(value);
}

Note that your string.IsNullOrEmpty() will never encounter a null reference for the string. Edit: after checking, I noticed that the SqlString.ToString() method converts a NULL SqlString to a literal value "Null", which is not what you want, I imagine. You should add this as the first thing to do in your method:

if (val.IsNull)
    return;

Lastly, returning to my complexity comment: if you're dealing with a large amount of elements, you might want to look at HashSet instead of using a List (same namespace). It won't maintain the order in which strings are added, but it's Contains operation is O(1), while your current one is O(n).

Thorarin