Internal global property..bad smell?

Question

I have run into a bit of a desgin issue with some code that I have been working on:

My code basic looks like this:

Main COM wrapper:

public class MapinfoWrapper 
{
    public MapinfoWrapper()
    {
        Publics.InternalMapinfo = new MapinfoWrapper();
    }

    public void Do(string cmd) 
    {
        //Call COM do command
    }

    public string Eval(string cmd)
    {
        //Return value from COM eval command
    }
}

Public static class to hold internal referance to wrapper:

internal static class Publics
{
    private static MapinfoWrapper _internalwrapper;
    internal static MapinfoWrapper InternalMapinfo 
    { 
        get
        {
            return _internalwrapper;     
        }
        set
        {
            _internalwrapper = value;
        }
    }
}

Code that uses internal wrapper instance:

    public class TableInfo
    {
        public string Name {
            get { return Publics.InternalMapinfo.Eval("String comman to get the name"); }
            set { Publics.InternalMapinfo.Do("String command to set the name"); }
        }
    }

Does this smell bad to anyone? Should I be using a internal property to hold a reference to the main wrapper object or should I be using a different design here?

Note: The MapinfoWrapper object will be used by the outside world, so I don't really want to make that a singleton.

Answer 1

You are reducing the testability of your TableInfo class by not injecting the MapInfoWrapper into the class itself. Whether you use a global cache of these MapInfoWrapper classes depends on the class -- you need to decide whether it is necessary or not, but it would improve your design to pass a wrapper into TableInfo and use it there rather than referencing a global copy directly inside TableInfo methods. Do this in conjunction with the definition of an interface (i.e., "refactor to interfaces").

I would also do lazy instantiation in the getter(s) of Publics to make sure the object is available if it hasn't already been created rather than setting it in the constructor of MapInfoWrapper.

public class TableInfo
{
     private IMapinfoWrapper wrapper;

     public TableInfo() : this(null) {}

     public TableInfo( IMapinfoWrapper wrapper )
     {
          // use from cache if not supplied, could create new here
          this.wrapper = wrapper ?? Publics.InternalMapInfo;
     }

     public string Name {
        get { return wrapper.Eval("String comman to get the name"); }
        set { wrapper.Do("String command to set the name"); }
     }
}

public interface IMapinfoWrapper
{
    void Do( string cmd );
    void Eval( string cmd );
}

public class MapinfoWrapper 
{
    public MapinfoWrapper()
    {
    }

    public void Do(string cmd) 
    {
        //Call COM do command
    }

    public string Eval(string cmd)
    {
        //Return value from COM eval command
    }
}

internal static class Publics
{
    private static MapinfoWrapper _internalwrapper;
    internal static MapinfoWrapper InternalMapinfo 
    { 
        get
        {
            if (_internalwrapper == null)
            {
                _internalwrapper = new MapinfoWrapper();
            }
            return _internalwrapper;     
        }
    }
}

Now, when you test the TableInfo methods, you can mock out the MapInfoWrapper easily by providing your own implementation to the constructor. Ex (assuming a hand mock):

[TestMethod]
[ExpectedException(typeof(ApplicationException))]
public void TestTableInfoName()
{
     IMapinfoWrapper mockWrapper = new MockMapinfoWrapper();
     mockWrapper.ThrowDoException(typeof(ApplicationException));

     TableInfo info = new TableInfo( mockWrapper );
     info.Do( "invalid command" );
}

Answer 2

I thought about adding this to my original response, but it is really a different issue.

You might want to consider whether the MapinfoWrapper class needs to be thread-safe if you store and use a cached copy. Anytime you use a single, global copy you need to consider if it will be used by more than one thread at a time and build it so that any critical sections (anywhere data may be changed or must be assumed to not change) are thread-safe. If a multithreaded environment must be supported -- say in a web site -- then this might argue against using a single, global copy unless the cost of creating the class is very high. Of course, if your class relies on other classes that are also not thread-safe, then you may need to make your class thread-safe anyway.