5
\$\begingroup\$

This is a short piece of code but I feel like it could be done more elegantly. What am I missing?

The goal: if there are any items in a list, and none of them have a given property, set one of them to have that property.

static void GuaranteeAtLeastOne<T>(IEnumerable<T> list, Func<T,bool> getter, Action<T> setter)
{
    if (list.Any() && !list.Any(getter))
    {
        setter(list.First());
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I think you've done the best you can. \$\endgroup\$
    – NetMage
    Aug 3 '11 at 19:12
4
\$\begingroup\$

I wouldn't assume to go for the First, rather I'd have a Func<T, bool> predicate parameter which you pass to a .FirstOrDefault(predicate) and ?? it with .First()

Also, I'd verify IEnumerable<T> != null before anything.

static void GuaranteeAtLeastOne<T>(IEnumerable<T> list, Func<T,bool> getter, Action<T> setter, Func<T,bool> predicate)
{
    if (list == null || list.Count == 0 || list.Any(getter))
    {
        return;
    }

    setter(list.FirstOrDefault(predicate) ?? list.First());
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Throwing an exception would be the correct behavior for a null argument. \$\endgroup\$
    – Ben Fulton
    Aug 2 '11 at 17:59
  • \$\begingroup\$ +1 @Ben: I agree ...instead of simply returning, should determine whether to throw NullArgument or InvalidArgument. \$\endgroup\$
    – IAbstract
    Aug 5 '11 at 17:41
  • \$\begingroup\$ @Ben I've gotten in the habit of working with nulls instead of exceptions lately to avoid the performance loss of exception bubbling, but that's just a style thing, however you want to handle the null I was merely suggesting to check for it :) \$\endgroup\$ Aug 9 '11 at 14:10
  • \$\begingroup\$ @Jimmy I still actually prefer my answer but you've made a lot of good points :) \$\endgroup\$
    – Ben Fulton
    Aug 11 '11 at 12:41
2
\$\begingroup\$

The one thing that you're doing is that you could potentially get the enumerator 3 times for any given sequence. If this is expensive and/or becomes an identifiable bottleneck, you may want to handle your checks in a single loop. You can do that in a foreach with a boolean flag, or you can access the enumerator directly. Whichever feels cleaner to you.

bool hasItem = false;
T first = null; // or perhaps ... = default(T);

foreach (T item in list)
{
    if (!hasItem)
    {
        hasItem = true;
        first = item;
    }

    if (getter(item))
        return; 
}

if (hasItem)
{
    setter(first);
}

Or using an enumerator and without a flag

using (var enumerator = list.GetEnumerator())
{
     if (!enumerator.MoveNext())
         return; 

     T first = enumerator.Current;

     do 
     {
         if (getter(enumerator.Current))
         {
             return;
         }
     } while (enumerator.MoveNext());

     setter(first);
}
\$\endgroup\$
0
\$\begingroup\$

I would separate the method into 3 distinct parts:

  1. Guard statement which checks if list is available
  2. A try to find an item that falls into required condition
  3. A setter which sets reuiqred property to an item

In that case 2nd and 3rd parts can be easily extracted from the method and be reused if needed.

static void GuaranteeAtLeastOne<T>(IEnumerable<T> list, Func<T,bool> getter, Action<T> setter)
{
  if (list == null || !list.Any())
  {
     return;
  }

  T item = list.Where(getter).FirstOrDefault();

  if (item != null)
  {
    return;
  }

  setter(list.First());
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Count() == 0 is inferior to Any() as it requires that the entire list be enumerated. \$\endgroup\$
    – Ben Fulton
    Aug 3 '11 at 21:04
  • \$\begingroup\$ Yes, a good comment. Then using Any() is better. \$\endgroup\$
    – Sergei
    Aug 4 '11 at 7:50
  • \$\begingroup\$ Count and Count() are different, Count is a cached property of a list, Count() is the linq extension which does the enumeration you referred to. The Count property is more clear and as O(1). \$\endgroup\$ Aug 4 '11 at 20:43
  • \$\begingroup\$ Count<T>() uses the ICollection<T> implementation for T that implements that interface, so it may not always require the entire list enumerated, but I think Any() is better since any good IEnumerable implementer should optimize that case. \$\endgroup\$
    – NetMage
    Aug 4 '11 at 22:14
  • \$\begingroup\$ In the question the interface of the list is IEnumerable which doesn't have Count property. So, we can use only Count() extension method, which is not as fast as using Any() method \$\endgroup\$
    – Sergei
    Aug 7 '11 at 12:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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