Making sure initialization code is executed before other code using Lazy Initialization

Question

The Problem

Recently I was told to refactor i.e. move some initialization logic from the constructor. I came up with something similar to the posted code. In actual there are bunch of loader classes inheriting from ILoader and doing all kind of stuff (TPL, DB access,XML parsing, CSV Writing) One of our UBER Developers reviewed my code and give some really appreciative comments particularly with the line of code marked with //<--:

Wow – I have seen a lot but that would make the http://thedailywtf.com/ for sure. HOW can you think of having a Getter method have a initialization SIDE EFFECT??

What is your opinion guys. What should I've done to avoid such appreciative comments?

The Solution

using System;

public class Program
{
    public static void Main()
    {
        var obj = new TestLoader("testing");
        Console.WriteLine("Calling Run() first time....");
        obj.Run();
        Console.WriteLine("Calling Run() again...");
        obj.Run();
        Console.WriteLine("Calling SomeOtherMethod()...");
        obj.SomeOtherMethod();
    }
}

//Class to make sure initialize will only be called once
public abstract class DataInitializer<T>
{
    public bool IsInitialized { get { return _isInitialized.Value; } }//<--
    readonly Lazy<bool> _isInitialized;
    protected DataInitializer(T param)
    {
        Console.WriteLine("Inside DataInitializer constructor");
        _isInitialized = new Lazy<bool>(Initialize);
    }
    protected abstract bool Initialize();
}

//Some loader class
public class TestLoader :DataInitializer<string>, ILoader
{
    public TestLoader(string test):base(test)
    {
    }

    protected override bool Initialize()
    {
        Console.WriteLine("Inside Initialize()");
        return true;
    }

    public void Run()
    {
        if(IsInitialized)
        {
            Console.WriteLine("Inside Run()");
        }
    }


    public void SomeOtherMethod()
    {
        if(IsInitialized)
        {
            Console.WriteLine("Inside SomeOtherMethod()");
        }
    }

}

interface ILoader
{
    void Run();
    void SomeOtherMethod();
}

Answer 1

Your code does feel like a misuse of lazy evaluation. Some alternative approaches to consider:

1. MakeInitialize method public and throw if it was not called:

   public override void Initialize()
   {
       //do stuff
       _isInitialized = true;
   }
   
   public void Run()
   {
       if(!_isInitialized) throw new InvalidOperationException();
       //do stuff
   }
   

   IMHO, it is all-around best option, as it allows you to control when to do initialization.

2. Call initialization automatically on every Run call:

   protected override void Initialize()
   {
       if (_isInitialized) return;
       //do stuff
       _isInitialized = true;
   }
   
   public void Run()
   {
       Initialize();
       //do stuff
   }
   

   This approach is good in some cases. For example, when you have an unstable network connection, it makes sense to check the connection before every method call, and reconnect if needed.

3. Use Lazy objects to initialize actual services:

   public CsvWriter CsvWriter { get { return _csvWriter.Value; } }
   readonly Lazy<CsvWriter> _csvWriter;
   
   public void Run()
   {
       CswWriter.Write(...)
   }
   

   I'm not a fan of lazy initialization, but it is fine, if you use it to instantiate one or two actual services. Laziness is justified in scenarios when there are high chances that Run method will not be called at all by your software.

P.S. Note, that Lazy is thread-safe, while options #1 and #2 are not and require synchronization if thread-safety is required.

Answer 2

You should listen to your collegue. This is a horrible idea. Let me quote MSDN about Properties (C# Programming Guide):

A property is a member that provides a flexible mechanism to read, write, or compute the value of a private field. Properties can be used as if they are public data members, but they are actually special methods called accessors. This enables data to be accessed easily and still helps promote the safety and flexibility of methods.

This means it should not have any side effects. Properties are expected to be cheap and their purpose is to access data (quickly). If an operation can take some time it should be implemented as a method. On the other hand the IsInitilized property would never return a false so it's actually useless too. You could never tell if an object is not-initialized.

Answer 3

Yes, it is an absolutely horrible idea to have the IsInitialized getter call Initialize()!

Aside from the options mentioned in NikitaB's answer, another option would be to wrap the Initialize() call in the base-class so you can set IsInitialized there:

public abstract class DataInitializer<T>
{
    public bool IsInitialized { get; private set; }
    protected abstract void InitializeImpl();

    public void Initialize()
    {
        if(!IsInitialized) // lock if thread-safety is required
        {
            InitializeImpl();
            IsInitialized = true;
        }
    }
}

I'm assuming DataInitializer<T> contains more code than this in real life - if this is literally all DataInitializer<T> does, you are basically just reimplementing Lazy<T> as a base-class.