views:

447

answers:

4

I've found this piece of code on Koders:

private ServiceProvider SiteServiceProvider
{
    get
    {
        if (serviceProvider == null)
        {
            serviceProvider = new ServiceProvider(site as VSOLE.IServiceProvider);
            Debug.Assert(serviceProvider != null, "Unable to get ServiceProvider from site object.");
        }
        return serviceProvider;
    }
}

I'm wondering, is there any possible way the Debug.Assert(serviceProvider != null could trigger? I'm under the impression that new could only be aborted by an exception, in which case the assert would never be reached.

+8  A: 

It's possible that the ServiceProvider overrides the !=/== operator, so that for an invalid state the comparison to null returns true.

Looks strange anyway.

arul
So true. *gag*
David Schmitt
+7  A: 

I would expect that "test for null" pattern more if it was a factory method - i.e.

SomeType provider = SomeFactory.CreateProvider();
if(provider == null) // damn!! no factory implementation loaded...
{ etc }

There is one other case worth knowing about, but which doesn't apply here (since we know the type we are creating)... Nullable<T>; this is mainly an issue with generics:

static void Test<T>() where T : new()
{
    T x = new T();
    if (x == null) Console.WriteLine("wtf?");
}
static void Main()
{
    Test<int?>();
}

This is covered more here.

Marc Gravell
Both interesting points (therefore +1), which - as you correctly assert - do not apply here.
David Schmitt
@David - it applies to the question title, though - hence why I included it ;-p
Marc Gravell
+3  A: 

I agree. If the normal != operator (inherited from Object) is being used, this can never happen. A constructor always returns an object reference and, as you have pointed out, if an exception was thrown in the constructor the execution point would leave the property entirely.

I would check what this code is intended to do. The constructor could, of course, have left the constructed object in an inconsistent state, and that is presumably what should have been tested for.

If your ServiceProvider class implements System.IServiceProvider, you may well want to check to make sure that GetService() does not return null.

andypaxo
A: 

What ever it is... it is definately a Code Smell regardless.

NathanE