Domestic global ownership ... smell?
I ran into a desgin problem with some code I was working on:
My main code 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 for storing the internal reference to the wrapper :
internal static class Publics
{
private static MapinfoWrapper _internalwrapper;
internal static MapinfoWrapper InternalMapinfo
{
get
{
return _internalwrapper;
}
set
{
_internalwrapper = value;
}
}
}
Code that uses the inner shell 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"); }
}
}
Is this smell bad for anyone? Should I use an internal property to store the reference to the main wrapper object or should I use a different design here?
Note. The MapinfoWrapper object will be used by the outside world, so I really don't want to make it a single.
source to share
You reduce the testability of your TableInfo class by not injecting MapInfoWrapper into the class itself. Whether you use the global cache of these MapInfoWrapper classes depends on the class - you need to decide if you need it or not, but it will improve your design to pass the wrapper to theInfo table and use it there, rather than referring to the global copy directly inside TableInfo methods. Do this in conjunction with an interface definition (ie "Refactor to interfaces").
I would also make a lazy instance in getter (s) Publics to make sure the object is available if not already created, rather than setting it in the MapInfoWrapper constructor.
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 that you are testing TableInfo methods, you can easily wear out MapInfoWrapper by providing your own implementation to the constructor. Ex (assuming manual layout):
[TestMethod]
[ExpectedException(typeof(ApplicationException))]
public void TestTableInfoName()
{
IMapinfoWrapper mockWrapper = new MockMapinfoWrapper();
mockWrapper.ThrowDoException(typeof(ApplicationException));
TableInfo info = new TableInfo( mockWrapper );
info.Do( "invalid command" );
}
source to share
I was thinking about adding this to my original answer, but this is really a different problem.
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 whether it will be used by more than one thread at a time, and structure it so that any critical sections (wherever the data that might be changed or should be accepted no change) is safe. If a multithreaded environment needs to be supported - say, on a website - then it might object to 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 might need to make your class thread safe.
source to share