Encapsulating an expensive resource without using a Singleton

I am working on updating a legacy application that is absolutely rife with Singleton classes. A perfect example would be the SnmpConnector class:

public SnmpConnector
{
  public static IEnumerable<string> HostIpAddresses
  {
    ...
  }

  private static SnmpConnector instance;
  public static SnmpConnector Instance
  {
    if (instance == null)
      instance = new SnmpConnector();
    return instance;
  }

  private SnmpConnector()
  {
    foreach (string IpAddress in HostIpAddresses)
    {
      ...
    }
  }

  ...
}

The goal of this update is to increase testability of the codebase, and as such I want to get rid of the Singletons. I've already abstracted away the data source of the SnmpConnector to either get dat开发者_开发百科a from a test database or from querying a live server:

public interface ISnmpDataSource
{
  public DataTable MacTable
  {
    get;
    private set;
  }

  public DataTable PortTable
  {
    get;
    private set;
  }

  ...
}

public TestSnmpDataSource : ISnmpDataSource
{
  public FileInfo DataSource
  {
    get;
    private set;
  }

  ...
}

public SnmpDataSource : ISnmpDataSource
{
  public List<string> HostIpAddresses
  {
    get;
    private set;
  }

  ...
}

public SnmpConnector
{
  public SnmpConnector(ISnmpDataSource DataSource)
  {
    ...
  }

  ...
}

Now, I'm trying to test these components and running into the problem that probably caused SnmpConnector to be a Singleton in the first place: it takes an ungodly amount of time to test the SnmpDataSource. It turns out that fetching the MAC table and Port table from live switches takes somewhere between 10 and 20 seconds. I've already written 13 unit tests for this particular class, so it takes over two minutes for just these tests to complete. As annoying as this is, it gets worse once these updates get published to our original codebase. With this new refactoring, there is nothing stopping a programmer from creating and discarding an SnmpDataSource repeatedly.

Now, the data from these tables is largely static; the old Singleton and the new SnmpDataSource both maintain a cache that was only updated every four hours. Will I have to make SnmpDataSource a Singleton to prevent this problem?

Use dependency injection, and either pass the SnmpDataSource into anything that needs it, or potentially pass in a Func<SnmpDataSource> which can create the instance lazily as necessary.

Is your goal that the SnmpDataSource should update itself, or that callers will get a new version after a few hours?

You could try wrapping/decorating the SnmpDataSource with a cache-aware version that implements the same interface, then inject the cache-aware version.

*edit -- or you could do what Jon suggested where the factory Func does the caching instead (it will return a new instance or a cached version depending on when the last one was created). Same thing, slightly different implementation. Jon's version probably makes more sense.

public CachedSnmpDataSource : ISnmpDataSource
{
  private DateTime m_lastRetrieved;
  private TimeSpan m_cacheExpiryPeriod;
  private List<string> m_hostIpAddresses;
  private Func<SnmpDataSource> m_dataSourceCreator

  public CachedSnmpDataSource(Func<SnmpDataSource> dataSourceCreator, TimeSpan cacheExpiryPeriod)
  {
      m_dataSourceCreator = dataSourceCreator;
      m_cacheExpiryPeriod = cacheExpiryPeriod;
  }

  public List<string> HostIpAddresses
  {
    get
    {
       if(!IsRecentCachedVersionAvailable()) 
       {
           CreateCachedVersion();
       }

       return new List<string>(m_hostIpAddresses);
    } 

    private bool IsRecentCachedVersionAvailable()
    {
        return m_hostIpAddresses != null && 
               (DateTime.Now - m_lastRetrieved) < m_cacheExpiryPeriod;
    }

    private void CreateCachedVersion()
    {  
       SnmpDataSource dataSource = m_dataSourceCreator();
       m_hostIpAddresses = dataSource.HostIpAddresses;
       m_lastRetrieved = DateTime.Now;       
    }
  }

  ...
}

After a couple of iterations, I've ended up with a neat solution to this problem. I'm going to leave the accepted answer as is, but this is what I ultimately used:

1. ISnmpDataSource is responsible for fetching data, as before.
2. The SnmpConnector knows to only query if its own cache is invalid.
3. A static Factory class maintains a Dictionary<ISnmpDataSource, SnmpConnector>. There is a static BuildSnmpConnector(ISnmpDataSource) method based on this dictionary.

Using the library now looks like this:

IEnumerable<string> IpAddresses = ...;
string SqlConString = @"...";
ISnmpDataSource Switches = new SnmpDataSource(IpAddresses, SqlConStr);
SnmpConnector instance = Factory. BuildSnmpConnector(Switches);

I had a few problems with how I implemented GetHashCode and Equals for the ISnmpDataSource implementations, but formalizing a definition of equality pretty much fixed all those problems.

I'm pretty happy with the final result; the Factory class is responsible for limiting instantiation, while the SnmpConnector is responsible for caching query results, while the ISnmpDataSource is responsible for actually running queries. I'm sure there is a better organization out there, but this one is clean enough for use.