2
\$\begingroup\$

I'm sick of writing negative code statements like String.IsNullOrEmpty(value) and so I'm writing a function that checks for availability. It should answer the question, "Is this value usable?" without validating the data.

This function has been exceptionally difficult to write but I have a solution I feel comfortable with using. I'm hoping someone can poke holes in it and give me constructive feedback.

public static bool IsAvailable<T>(T value)
{
    if (value == null)
        return false;

    var type = typeof(T);
    if (type.IsValueType)
        return true;

    if (typeof(IEnumerable).IsAssignableFrom(type))
    {
        var enumerator = (value as IEnumerable).Cast<object>();
        return enumerator.Any(a => IsAvailable(a));
    }

    var valueString = Convert.ToString(value);
    return !String.IsNullOrWhiteSpace(valueString);
}
\$\endgroup\$
1
  • \$\begingroup\$ I noticed that I don't account for value types that have obviously wrong default values. For example, defulat(Guid) and default(DateTime). It would be nice to know if there are any other built in structs I should account for. \$\endgroup\$ – christo8989 Sep 21 '17 at 20:06
5
\$\begingroup\$

I'm not so sure about this method:

  • Why would you call this for a value type, if you already know that you're dealing with a value type? You mentioned something about 'obviously wrong default values', but that probably depends on the context.
  • If you do find a general definition of what exactly being 'available' means, you may want to document it. Chances are that what's obvious to you isn't obvious to someone else, or even to yourself a few months later.
  • What's the point of checking if an IEnumerable contains Any available items? It could still contain many unavailable items, so you'd still have to check each item before use. All would be more appropriate, but that would make this method very 'eager'.
  • Iterating an IEnumerable can cause problems when you're working with lazily evaluated sequences. You'd have to be careful with where you use this method, or you could accidentally fetch data from a database twice, or do other duplicated work. Letting the caller check each item before use seems a much safer (and cheaper) alternative.
  • That Cast<object>() call causes T in the inner call to be object, so the IsValueType and IEnumerable checks won't work.
  • You'll want to know the actual type of the value you're checking, so use value.GetType() instead of typeof(T). And instead of calling IsAssignableFrom, you can use is, or just use as immediately and then check if the result isn't null.
  • What's the point of converting the given value/object to a string? It's basically checking the result of value.ToString(). Any class that overrides ToString and that returns null or an empty string will always be seen as unavailable, which is probably a subtle bug lying in wait. I'm not sure what this check is meant to accomplish.

All in all, I don't really see what value this method provides. If you've got a value type then you probably already know it, and you most likely know better which values are valid and which ones aren't, depending on context. In any other case, you can use the Elvis operator (?.) or do a null check of some sort.

\$\endgroup\$
3
  • \$\begingroup\$ I'm using it as a private method in a class that acts like Contracts or Assertions which will throw the specified exception on false. Instead of if (expression) throw new Exception("Blah"); everywhere, it reads like MustBe.Available(value, "Blah");. I mostly find it easier to read and saves time when coding. But I wish writing the IsAvailable method was easier. But you're definitely right. :( \$\endgroup\$ – christo8989 Sep 21 '17 at 21:10
  • 4
    \$\begingroup\$ It might be a little easier then to write several more specific methods, such as MustBe.NotNull<T>(T obj) where T : class, MustBe.NotEmpty(IEnumerable enumerable), and so on. It'll also be more obvious just what exactly they guarantee. \$\endgroup\$ – Pieter Witvoet Sep 21 '17 at 21:21
  • 1
    \$\begingroup\$ This all-in-one method definitely should be split into multiple ones. \$\endgroup\$ – Maxim Sep 22 '17 at 1:53

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.