3
\$\begingroup\$

Given these two methods, is it defensible that one is better than the other? In what terms? Performance? Elegance? Readability?

public static void AddFirstNotNull(List<object> onlyNotNull, List<object> source)
{
  var item = source.FirstOrDefault(v => v != null);
  if (item != null)
  {
    onlyNotNull.Add(item);
  }
}

public static void AddFirstNotNull(List<object> onlyNotNull, List<object> source)
{
  onlyNotNull.AddRange(source.Where(v => v != null).Take(1));
}
\$\endgroup\$
  • \$\begingroup\$ I find the first method more understandable as to the method's intent. \$\endgroup\$ – Rick Davin Mar 2 '16 at 14:41
4
\$\begingroup\$

I agree with @RickDavin that the first function expresses its intent in a more obvious way.

My main concern, though, is that neither function provides feedback as to whether the operation actually resulted in any change. The function should return a boolean. It's easy to modify the first function to return a boolean, but not the second.

\$\endgroup\$
3
\$\begingroup\$

Concerning performance, i'd say that any difference, if it exists, is negligible. In this case both elegance and readability are bound to your subjective feeling, both are completely fine and are used all the time. The only thing you could do is to not mix them in a solution. Stick with one to the end.

\$\endgroup\$
3
\$\begingroup\$

I'm mixed about helpers like this that cannot be named in a way that makes it clear what they do.

The signature: public static void AddFirstNotNull(List<object> onlyNotNull, List<object> source)

  1. Does this really have to be public? Feels like a small helper with very local scope. For public methods you should validate inputs and throw if source or target is null.
  2. I prefer the source argument as IEnumerable<object> to make it clear that it is not changed.

Perhaps make it generic?:

public static void AddFirstNotNull<T>(IEnumerable<T> source, List<T> target)

An alternative could be an extension method for List<T> (Using @200_success's suggestion and returning a bool)

public static bool AddIfNotNull<T>(this List<T> list, T item)
    where T : class 
{
    Ensure.NotNull(list, nameof(list));
    if (item == null)
    {
        return false;
    }

    list.Add(item);
    return true;
}
\$\endgroup\$

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.