current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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?

share|improve this question
    
I'm not sure what the title of this question has to do with the content. You might consider revising it. –  John Kraft Jul 19 '11 at 13:24
    
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 :) –  Jaymz Jul 19 '11 at 14:49

3 Answers 3

up vote 1 down vote accepted

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;
}
share|improve this answer
1  
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. –  Jaymz Jul 20 '11 at 10:39
1  
More efficient alternative: actions.Distinct().Take(2).Count() == 1. –  mgronber Jul 20 '11 at 23:13
up vote 0 down vote

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.

share|improve this answer
    
You missed the “.Distinct()”. –  Timwi Jul 19 '11 at 13:30
1  
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. –  Jaymz Jul 19 '11 at 14:52
1  
Just out of curiosity. If there should only be one action available, why is it using a list of actions? –  John Kraft Jul 19 '11 at 15:50
    
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. –  Jaymz Jul 20 '11 at 10:25
up vote -1 down vote

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();
share|improve this answer
1  
Your code will give different result for Count() == 1 –  Snowbear Jul 19 '11 at 14:08
    
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...? –  Jaymz Jul 19 '11 at 14:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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