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

Question

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.

One reason I could think of: somewhere down the road, you want to make it static and forget about it in the constructor and now everytime a new object gets created, the static Property gets overridden. — Corak, May 22 '13 at 13:23
The only reason I could see for this check if there are method calls above this line in the constructor that could set the repository. (I realise there isn't in your example) — Sayse, May 22 '13 at 13:24
Is the `DocumentRepository` property an autoproperty exactly like you wrote it in your question? If so then it is indeed pointless. If not, then we can't answer without seeing the real implementation. — Matthew Watson, May 22 '13 at 13:24
It's possible that the "constructor" you are recalling was actually a static constructor, (Initializer), or perhaps it was a static factory method. If so, this could have been be part of a standard singleton pattern. — Charles Bretana, May 22 '13 at 13:25
FYI - when you say "class variable" (or member) the "class" qualifier is frequently understood to mean the same thing as `static` (as opposed to "instance"). I point this out because this kind of check-and-initialize-if-necessary logic is not unusual for static members. — David, May 22 '13 at 13:39
I think original author of that code may have been confused by object initializer syntax `DocumentService d = new DocumentService { DocumentRepository = xyz };` — YK1, May 22 '13 at 16:38

score 17 · Accepted Answer · answered May 22 '13 at 13:21

17

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.

answered May 22 '13 at 13:21

H H

263,252
30
330
514

3

@DoctorOreo: I would suggest that if you do take the unnecessary check out, you consider `Debug.Assert(DocumentRepository == null); DocumentRepository = new DocumentRepository();` -- that way if the check really *was* sensible in some bizarre scenario that you don't know about, you'll soon know about it. – Eric Lippert May 22 '13 at 17:59
The assertion suggested by @EricLippert is also a way for the writer to document his/her expectations of the behavior of the code. And that documentation can be informative for people who read the code in the future. – Clayton Stanley May 22 '13 at 18:43

score 12 · Answer 2 · answered May 22 '13 at 16:24

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.

I really appreciate that someone with a great deal of knowledge and influence has recognized this practice and written about it. It's a great source of irritation when I ask a colleague "why did you do this" and the answer is "I don't know, I copied it from somewhere else." — JDB, May 22 '13 at 16:32
@Cyborgx37: *Why do you always set your references to null?* Because it keeps the alligators away. *But there are no alligators in Seattle.* See how well it works? — Eric Lippert, May 22 '13 at 17:49
Of course, every once in a while when I step out of line to remove such silly fluff, "[the payroll data gets deleted](http://dilbert.com/strips/comic/2009-08-30/)". — JDB, May 22 '13 at 17:53

score 5 · Answer 3 · answered May 22 '13 at 13:23

5

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

answered May 22 '13 at 13:23

Dennis Traub

50,557
7
93
108

score 5 · Answer 4 · answered May 22 '13 at 13:29

5

May be the == operator for DocumentRepository is overwritten.

answered May 22 '13 at 13:29

Chris

1,610
3
18
37

How in the world do you override an operator? – Cody May 22 '13 at 13:29
1

@DoctorOreo For an example, see my answer in this thread: http://stackoverflow.com/a/16655690/106159 That shows you how to overload ==, but it also shows you why you must also override `Equals()` if you do so. :) – Matthew Watson May 22 '13 at 13:32
Wow, great references. My initial reaction was "wut", but that's really cool. Didn't know you could do that. Learn something new everyday, thanks SO!! – Cody May 22 '13 at 13:35

Damien_The_Unbeliever · Answer 5 · 2013-05-23T08:07:50.230

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
  } 

}

Microsoft's own Code Contracts moves the contract checks to before the call to the base constructor, IIRC. (It uses a binary rewriter, but I think this is a case where the post-compilation step isn't weird.) And it doesn't check whether the contracts have side effects. — , May 23 '13 at 06:45

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

5 Answers5