2
\$\begingroup\$

I'm just refactoring some code I've come across whilst fixing a bug, but just wanted a quick sanity check to make sure I'm not missing any reason for keeping what I'm going to remove.

The current code:

uxButton.Enabled = ((from a in actions select a).Distinct().Count() == 1);

There really doesn't seem to be a need for the LINQ query, so:

uxButton.Enabled = actions.Distinct().Count() == 1;

Should do the trick, right?

\$\endgroup\$
2
  • \$\begingroup\$ I'm not sure what the title of this question has to do with the content. You might consider revising it. \$\endgroup\$
    – John Kraft
    Commented Jul 19, 2011 at 13:24
  • \$\begingroup\$ You may well be right, I was a bit unsure of a title - basically what I meant was that the LINQ query is a select which returns the original set of objects... if that makes sense? If you can think of a better title, I'll happily change it :) \$\endgroup\$
    – Jaymz
    Commented Jul 19, 2011 at 14:49

3 Answers 3

2
\$\begingroup\$

As the other answers stated your given code does do the same thing, but could give performance issues for big collections since the entire collection is traversed when calling Count().

The following code should solve this:

var distinct = actions.Distinct();
uxButton.Enabled = distinct.Take( 2 ).Count() == 1;

Perhaps you could turn this into an extension method:

uxButton.Enabled = actions.Distinct().CountOf( 1 );

It seemed useful, so I added it to my library, along with a unit test.

/// <summary>
///   Returns whether the sequence contains a certain amount of elements.
/// </summary>
/// <typeparam name = "T">The type of the elements of the input sequence.</typeparam>
/// <param name = "source">The source for this extension method.</param>
/// <param name = "count">The amount of elements the sequence should contain.</param>
/// <returns>
///   True when the sequence contains the specified amount of elements, false otherwise.
////</returns>
public static bool CountOf<T>( this IEnumerable<T> source, int count )
{
    Contract.Requires( source != null );
    Contract.Requires( count >= 0 );

    return source.Take( count + 1 ).Count() == count;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice, I like this. I've added this method to our project now, but have called it CountIs - collection.CountIs(1) sounds more logical than collection.CountOf(1) to me, seeing as it's returning a bool. Thanks. \$\endgroup\$
    – Jaymz
    Commented Jul 20, 2011 at 10:39
  • 1
    \$\begingroup\$ More efficient alternative: actions.Distinct().Take(2).Count() == 1. \$\endgroup\$
    – mgronber
    Commented Jul 20, 2011 at 23:13
0
\$\begingroup\$

They both do the same thing. The first one just looks bizarre. Looks like the original author just didn't understand Linq.

I'm not sure why you are checking to see if the count is 1. Looks like, in this situation

uxButton.Enabled = actions.Distinct().Any()

would be more appropriate and more performant.

\$\endgroup\$
4
  • \$\begingroup\$ You missed the “.Distinct()”. \$\endgroup\$
    – Timwi
    Commented Jul 19, 2011 at 13:30
  • 1
    \$\begingroup\$ I need the Count to be 1, as if it's more than 1, it should be disabled. Otherwise, I would have used Any. But thanks for confirming my thoughts. \$\endgroup\$
    – Jaymz
    Commented Jul 19, 2011 at 14:52
  • 1
    \$\begingroup\$ Just out of curiosity. If there should only be one action available, why is it using a list of actions? \$\endgroup\$
    – John Kraft
    Commented Jul 19, 2011 at 15:50
  • \$\begingroup\$ There can be more than one action in the list of actions, but the button can only be enabled if there is 1 action in the list. For all other counts, it should be disabled. \$\endgroup\$
    – Jaymz
    Commented Jul 20, 2011 at 10:25
-1
\$\begingroup\$

Yes, you are right, the LINQ query is superfluous and you can just write actions instead.

The only comment I’d have about the code is that .Distinct().Count() will go through the entire collection, but you can stop when a non-distinct element is found. So personally I’d use this instead:

uxButton.Enabled = actions.Distinct().Skip(1).Any();
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your code will give different result for Count() == 1 \$\endgroup\$
    – Snowbear
    Commented Jul 19, 2011 at 14:08
  • \$\begingroup\$ Won't this return true for a collection of more than 1? I need it to be false for anything other than 1, hence the check for Count() == 1; Unless there's another method which returns true only for collections with a single element, I believe Count() to be the only option...? \$\endgroup\$
    – Jaymz
    Commented Jul 19, 2011 at 14:55

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.