magnattic
magnattic

Reputation: 13028

Is checking for type instead of null a reasonable choice?

One of my (senior) coworkers does something really strange in his code.

Instead of checking a variable for null, he checks for the type. And because

null is FooType

actually returns false, this works.

public class Foo
{
    private string _bar = null;

    public string Bar
    {
        get
        {
            // strange way to check for null
            return (_bar is string) ? _bar : "";
        }
        set { _bar = value; }
    }
}

I think this is bad coding and Resharper seems to agree with me. Is there any reason to write the check this way?

Is this a valid way to check the variable? Or can this be considered bad style or maybe even harmful in some special cases?

I don't want to confront him unless I am sure that this actually does not make sense.

Upvotes: 5

Views: 167

Answers (4)

asawyer
asawyer

Reputation: 17808

Yeah that's a little odd. Why not just write:

return _bar ?? "" ;

When I need to do something like this, I have a little class to handle these details:

public class DefaultableValue<T>
{
    private T m_Value = default(T);
    public T Value
    {
        get
        {
            if (IsInvalidPredicate(m_Value))
            {
                m_Value = IfDefaultValueFunc();
            }
            return m_Value;
        }
    }
    private Predicate<T> IsInvalidPredicate { get; set; }
    private Func<T> IfDefaultValueFunc { get; set; }
    public static implicit operator T(DefaultableValue<T> property)
    {
        return property.Value;
    }
    public DefaultableValue(Predicate<T> isInvalidPredicate,Func<T> ifDefaultFunc)
        : this(default(T), isInvalidPredicate, ifDefaultFunc)
    {
    }
    public DefaultableValue(T initValue, Predicate<T> isInvalidPredicate, Func<T> ifDefaultFunc)
    {
        this.m_Value = initValue;
        this.IsInvalidPredicate = isInvalidPredicate;
        this.IfDefaultValueFunc = ifDefaultFunc;
    }
}

Then my class looks like

class Test
{
    DefaultableValue<string> AString { get; set; }

    public Test(string initialValue)
    {
        this.AString = new DefaultableValue<string>(initialValue, 
            (value) => string.IsNullOrWhiteSpace(value),
            () => string.Empty);
    }
}

....
var test = new Test(null);
var someString = test.AString; // = "" not null

Upvotes: 3

Huske
Huske

Reputation: 9296

if the above public property was declared to return object instead of string, the above could make sense. However, because it is returning string, that type of check does not make sense. If you want to return an empty string that you could do something like this:

public class Foo 
{ 
    private string _bar = null; 

    public string Bar 
    { 
        get 
        {  
            return (String.IsNullOrWhitespace(_bar)) ? "": _bar; 
        } 
        set { _bar = value; } 
    } 
} 

Upvotes: 0

Kirk Woll
Kirk Woll

Reputation: 77606

I think this code is completely confusing and would never use it. _bar is declared as a string so this type check is just begging people to not understand the code.

Upvotes: 3

Oskar Kjellin
Oskar Kjellin

Reputation: 21900

This is not a good way. A better way would be to just:

return _bar ?? string.Empty;

Is it clear when you read your colleagues code that he is looking for nulls? No, then it isn't a good choice. Probably what the "is" operator will do first is just check for null and then return false. So it becomes much cleaner to just do that yourself. Or just use the null-coalescing operator

Upvotes: 10

Related Questions