Probably BAD coding style ... please comment

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 :-/

From stackoverflow SilverHorse

• 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.

  Jon Skeet : 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.

  jeffm : 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.
  From jeffm

• I like the first one better.

  From Burkhard

• 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.

  From Georgi

• 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.

  From Fry

• 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.

  From EBGreen

• 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

  Georgi : Erm, you mean the "first" ?

  1800 INFORMATION : That's what I said, what are you talking about? ;)
  From 1800 INFORMATION

• would

  cmbxExistingGroups.Properties.Items.Contains(text)
  

  not work instead?

  FlySwat : Contains would work, considering he using a String and not a complex object.

  Jon Skeet : 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.
  From Steven A. Lowe

• 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.

  From FlySwat

• 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.

  SilverHorse : hi jon ,..what about the implementation that Steven A. Lowe gave?

  SilverHorse : cmbxExistingGroups.Properties.Items.Contains(text)

  Jon Skeet : 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 : Ah, but I see there's also an "extra extension method" version - that works very nicely.
  From Jon Skeet

• 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.

  FlySwat : The first and second are both O(N) operations, performance is the same.

  FlySwat : Excluding the extra iteration that ToList() requires in the first one, but that is an unnecessary call.
  From gbarry

• 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.

  Jon Skeet : 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.)

  mattlant : You make a good point, thx.
  From mattlant

• 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;
      }
  }
  

  From Robert Paulson

• 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.

  Jon Skeet : 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 : (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.

  Mark Brackett : 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. Which makes the Cast unnecessary.
  From Mark Brackett

• 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");
  }
  

  From Alex Lyman

• 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.

  From David B

• 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.

  SilverHorse : doing wrong by askign for comments?
  From Coderer