0

I am checking whether the new name already exists or not.

Code 1

if(cmbxExistingGroups.Properties.Items.Cast<string>().ToList().Exists(txt => txt==txtNewGroup.Text.Trim())) {
        MessageBox.Show("already exists.", "Add new group");
      }

Otherwise I could have written:

Code 2

foreach(var str in cmbxExistingGroups.Properties.Items)
      {
        if(str==txtNewGroup.Text) {
        MessageBox.Show("already exists.", "Add new group");        
            break;
        }   
      }

I wrote these two and thought I was exploiting language features in code 1.

...and yes: both of them work for me ... I am wondering about the performance :-/

2
  • basically i m using DevXpress user control. Its comboboxEdit control contains * Properties.Items *
    – Prerak K
    Oct 2, 2008 at 20:49
  • Good guideline: if you need more comments to explain what you did, than to write it in a simpler way, you're doing it wrong.
    – Coderer
    Oct 8, 2008 at 20:57

14 Answers 14

15

I appreciate the cleverness of the first sample (assuming it works), but the second one is a lot easier for the next person who has to maintain the code to figure out.

2
  • Do you think it's easier than the "Contains" version though? I certainly don't. The first version in the original question has various flaws, but the "express the question declaratively" aspect is fine, IMO.
    – Jon Skeet
    Oct 1, 2008 at 22:27
  • The "Contains" version hadn't been suggested at the time I wrote this, and the performance aspect hadn't been added to the question yet either.
    – jeffm
    Oct 2, 2008 at 4:44
11

Sometimes just a little indentation makes a world of difference:

if (cmbxExistingGroups.Properties.Items
    .Cast<string>().ToList()
    .Exists
     (
         txt => txt==txtNewGroup.Text.Trim()
     )) 
{
    MessageBox.Show("already exists.", "Add new group");
}

Since your using a List<String>, you might as well just drop the Exists predicate and use Contains...use Exists when comparing complex objects by unique values.

6

I've quoted it before but I'll do it again:

Write your code as if the person maintaining it is a homicidal maniac who knows where you live.

3

would

cmbxExistingGroups.Properties.Items.Contains(text) 

not work instead?

2
  • Contains would work, considering he using a String and not a complex object.
    – FlySwat
    Oct 1, 2008 at 21:15
  • It depends on the type of the Items property, surely. I can't find a ComboBox class with a "Properties" property, so I don't know exactly which type is involved.
    – Jon Skeet
    Oct 1, 2008 at 22:29
3

There are a few things wrong here:

1) The two bits of code don't do the same thing - the first looks for the trimmed version of txtNewGroup, the second just looks for txtNewGroup

2) There's no point in calling ToList() - that just make things less efficient

3) Using Exists with a predicate is overkill - Contains is all you need here

So, the first could easily come down to:

if (cmbxExistingGroups.Properties.Items.Cast<string>.Contains(txtNewGroup.Text))
{
    // Stuff
}

I'd probably create a variable to give "cmbxExistingGroups.Properties.Items.Cast" a meaningful, simple name - but then I'd say it's easier to understand than the explicit foreach loop.

4
  • hi jon ,..what about the implementation that Steven A. Lowe gave?
    – Prerak K
    Oct 1, 2008 at 21:29
  • cmbxExistingGroups.Properties.Items.Contains(text)
    – Prerak K
    Oct 1, 2008 at 21:31
  • That depends on the type of cmbxExistingGroups.Properties.Items. I can't easily tell it from the code presented here. If it only implements IEnumerable, then that won't compile. What's the type?
    – Jon Skeet
    Oct 1, 2008 at 22:10
  • Ah, but I see there's also an "extra extension method" version - that works very nicely.
    – Jon Skeet
    Oct 1, 2008 at 22:12
3

The first code bit is fine, except instead of calling Enumerable.ToList() and List<T>.Exists(), you should just call Enumerable.Any() -- it does a lazy evaluation, so it never allocates the memory for the List<T>, and it will stop enumerating cmbxExistingGroups.Properties.Items and casting them to string. Also, calling the trim from inside that predicate means it happens for every item it looks at. It would be best to move it out to the outer scope:

string match = txtNewGroup.Text.Trim();
if(cmbxExistingGroups.Properties.Items.Cast<string>().Any(txt => txt==match)) {
    MessageBox.Show("already exists.", "Add new group");
}
2

First, they're not equivalent. The 1st sample does a check against txtNewSGroup.Text.Trim(), the 2nd omits trim. Also, the 1st casts everything to a string, whereas the second uses whatever comes out of the iterator. I assume that's an object, or you wouldn't have needed the cast in the 1st place.

So, to be fair, the closest equivalent to the 2nd sample in the LINQ style would be:

if (mbxExistingGroups.Properties.Items.Cast<string>().Contains(txtNewGroup.Text)) {
   ...
}

which isn't too bad. But, since you seem to be working with old style IEnumerable instead of new fangled IEnumerable<T>, why don't we give you another extension method:

public static Contains<T>(this IEnumerable e, T value) {
   return e.Cast<T>().Contains(value);
}

And now we have:

if (mbxExistingGroups.Properties.Items.Contains(txtNewGroup.Text)) {
   ...
}

which is pretty readable IMO.

3
  • Eek - the "the second uses whatever comes out of the iterator" is actually a major problem - because if that's "object" at compile time, then the equality check being performed is actually just an identity check. So much for the second version being much clearer! (Continued)
    – Jon Skeet
    Oct 1, 2008 at 22:14
  • (Continued from previous.) However, this problem is caused by "var" - if you replace "var" with "string" then the compiler inserts an invisible cast for you on each iteration, and the comparison then uses the overloaded == operator, which is likely to be the intention.
    – Jon Skeet
    Oct 1, 2008 at 22:15
  • Good point - unless they're interned or cloned. Considering this code supposedly worked, my guess is now that it's not IEnumerable at all, and instead IEnumerable<string>. Which makes the Cast<T> unnecessary. Oct 2, 2008 at 15:53
1

Verbosity in coding is not always bad at all. I prefer the second code snippet a lot over the first one. Just imagine you would have to maintain (or even change the functionality of) the first example... um.

1

both of them works for me ..i am wonodering about the performance

I see no one read the question :) I think I see what you're doing (I don't use this language). The first tries to generate the list and test it in one shot. The second does an explicit iteration and can "short circuit" itself (exit early) if it finds the duplicate early on. The question is whether the "all at once" is more efficient due to the language implementation.

2
  • The first and second are both O(N) operations, performance is the same.
    – FlySwat
    Oct 1, 2008 at 21:45
  • Excluding the extra iteration that ToList() requires in the first one, but that is an unnecessary call.
    – FlySwat
    Oct 1, 2008 at 21:46
1

Well, if it were me, it would be a variation on 2. Always prefer readability over one-liners. Additionally, always extract a method to make it clearer.

your calling code becomes

if( cmbxExistingGroups.ContainsKey(txtNewGroup.Text) )
{
   MessageBox.Show("Already Exists");
}

If you define an extension method for Combo Boxes

public static class ComboBoxExtensions
{
    public static bool ContainsKey(this ComboBox comboBox, string key)
    {
        foreach (string existing in comboBox.Items)
        {
            if (string.Equals(key, existing))
            {
                return true;
            }
        }
        return false;
    }
}
0

I would agree, go with the second one because it will be easier to maintain for anybody else who works on it and when you come back to that in 6-12 months, it will be easier to remember what you were doing.

0

The second of the two would perform better, and it would perform the same as other people's samples that use Contains.

The reason why the first one uses an extra trim. plus a conversion to list. so it iterates once for conversion, then starts again to check using exists, and does a trim each time, but will exit iteration if found. The second starts iterating once, has no trim, and will exit if found.

So in short the answer to your question is the second performs much better.

1
  • Contains might actually work faster than the second version - after all, it only fetches the value from the textbox once as opposed to every iteration. (Easy to fix in the foreach version of course.)
    – Jon Skeet
    Oct 1, 2008 at 22:11
0

From a performance point of view:

txtNewGroup.Text.Trim()

Do your control interaction/string manipulation outside of the loop - one time, instead of n times.

-1

I imagine that on the WTF's per minute scale, the first would be off the chart. Count the dots, any more than two per line is a potential problem

1
  • That's what I said, what are you talking about? ;) Oct 1, 2008 at 21:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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