CODEWITHSUNDEEP
c#.netconstructor
Cody
Cody

Reputation: 8944

Why check if a class variable is null before instantiating a new object in the constructor?

With a previous team I worked with, whenever a new Service class was created to handle business logic between the data layer and presentation layer, something like the following was done:

class DocumentService
{
    public DocumentRepository DocumentRepository { get; set; }

    public DocumentService()
    {
         if (DocumentRepository == null) DocumentRepository = new DocumentRepository();
    }
}

I never quite understood why the check for null was there. If the constructor is being called, that means it HAS to be null..since it's a new instance, right?

Why would this be done? It seems to me like it is a redundant step, but I don't want to miss anything and pass it off as bad practice.

Upvotes: 14

Views: 2195

Answers (5)

Damien_The_Unbeliever
Damien_The_Unbeliever

Reputation: 239724

There is one scenario I can think of where this almost makes sense - but you'd have to be working in an environment where C# isn't the only language (or you're doing weird post-compilation steps, such as might be possible with Postsharp).

In C# or VB.Net, you can't control when the base class constructor is called, and there's no syntax to allow you to set base class members before the base class constructor is called - but there's nothing to prevent it in IL.

If you were working in such an environment, and the constructor you've shown actually does anything further with DocumentRepository, then you might be setting things up correctly for the following class:

public class IllegalCSharpClass : DocumentService
{
    public IllegalCSharpClass()
    {
        DocumentRepository = new DocumentRepository("B");
        base(); //This is illegal C#, but allowed in IL
    }
}

I wouldn't want to work in such a workplace though.

Here's the IL for the class:

.class public auto ansi beforefieldinit PlayAreaCS_Con.IllegalCSharpClass
       extends PlayAreaCS_Con.DocumentService
{
  .method public hidebysig specialname rtspecialname 
          instance void  .ctor() cil managed
  {
    .maxstack  8
    IL_0008:  ldarg.0
    IL_0009:  ldstr      "B"
    IL_000e:  newobj     instance void PlayAreaCS_Con.DocumentRepository::.ctor(string)
    IL_0013:  call       instance void PlayAreaCS_Con.DocumentService::set_DocumentRepository(class PlayAreaCS_Con.DocumentRepository)
    IL_0018:  nop
    IL_0019:  nop
    IL_0000:  ldarg.0
    IL_0001:  call       instance void PlayAreaCS_Con.DocumentService::.ctor()
    IL_0006:  nop
    IL_0007:  nop
    IL_001a:  ret
  } 

}

Upvotes: 1

Eric Lippert
Eric Lippert

Reputation: 660159

Henk's answer is of course correct; I just wanted to add that I have a suspicion as to where this came from. I'll bet that at some point in the past someone wrote:

class DocumentService
{
    private DocumentRespository documentRespository = null;
    public DocumentRepository DocumentRepository 
    { 
        get
        {
            if (documentRepository == null) 
                documentRepository = new DocumentRepository();
            return documentRepository;
        }
    }
    public DocumentService()
    {
    }
}

That is, the property is initialized on its first use. Later someone realized that this was wrong or unnecessary, or whatever, and rewrote the code to put the construction in the constructor, making the property "eager" rather than "lazy", but forgetting to take out the null check.

If people then program by cutting and pasting from existing code to new code, the pattern can spread. And pretty soon a myth springs up that this is how it has to be done. This is why, I suspect, so many Visual Basic programmers have the false belief that you have to say Set Foo = Nothing when you are done with Foo; it was necessary once in some scenario, and now people do it even when it is not necessary because that's just how we do it here.

Incidentally, you probably want to say:

public DocumentRepository DocumentRepository { get; private set; } // note the private

It seems unlikely that you want users of your service to be able to change the repository on the fly.

Upvotes: 12

Chris
Chris

Reputation: 1642

May be the == operator for DocumentRepository is overwritten.

Upvotes: 5

Dennis Traub
Dennis Traub

Reputation: 51644

As far as I can tell, you're absolutely right. There is no need to check for null.

Upvotes: 5

Henk Holterman
Henk Holterman

Reputation: 273284

In this exact context: Yes it is redundant.

There is no immediate reason for this code, it could be a left-over from an older method or anticipating on an implementation with multiple constructors. But I would not recommend using this 'pattern' or even keeping this code.

Upvotes: 17

Related Questions