I have a singleton that anchors together some different data structures. Part of what I expose through that singleton are some lists and other objects, which represent the necessary keys or columns in order to connect the linked data structures. I doubt that anyone would try to change these objects through a different module, but I want to explicitly protect them from that risk. So I'm currently using a "readonly" modifier on those objects*.
I'm using readonly instead of const with the lists as I read that using const will embed those items in the referencing assemblies and will therefore trigger a rebuild of those referencing assemblies if / when the list(s) is/are modified. This seems like a tighter coupling than I would want between the modules, but I wonder if I'm obsessing over a moot point. (This is question #2 below)
The alternative I see to using "readonly" is to make the variables private and then wrap them with a public get. I'm struggling to see the advantage of this approach as it seems like wrapper code that doesn't provide much additional benefit. (This is question #1 below)
It's highly unlikely that we'll change the contents or format of the lists – they're a compilation of things to avoid using magic strings all over the place. Unfortunately, not all the code has converted over to using this singleton's presentation of those strings.
Likewise, I don't know that we'd change the containers / classes for the lists. So while I normally argue for the encapsulations advantages a get wrapper provides, I'm just not feeling it in this case.
A representative sample of my singleton
public sealed class mySingl
{
private static volatile mySingl sngl;
private static object lockObject = new Object();
public readonly Dictionary<string, string> myDict = new Dictionary<string, string>()
{
{"I", "index"},
{"D", "display"},
};
public enum parms
{
ABC = 10,
DEF = 20,
FGH = 30
};
public readonly List<parms> specParms = new List<parms>()
{
parms.ABC,
parms.FGH
};
public static mySingl Instance
{
get
{
if(sngl == null)
{
lock(lockObject)
{
if(sngl == null)
sngl = new mySingl();
}
}
return sngl;
}
}
private mySingl()
{
doSomething();
}
}
Questions:
- Am I taking the most reasonable approach in this case?
- Should I be worrying about const vs. readonly?
- is there a better way of providing this information?
To address some additional concerns:
First off, I understand there are folk who oppose the use of singletons. I think it's an appropriate use in this case as it's constant state information, but I'm open to differing opinions / solutions. (See The singleton pattern and When should the singleton pattern not be used?)
Second, for a broader audience: C++/CLI has a similar keyword to readonly
with initonly
, so this isn't strictly a C# type question.
(Literal field versus constant variable in C++/CLI)
Sidenote: A discussion of some of the nuances on using const or readonly.
Best Answer
Given your code, I have two concerns.
First, readonly references to mutable child objects (like collections) do not make the properties of the child immutable.
If you want a truly immutable child object, you have to make the reference immutable AND make sure the object itself is immutable. For that, you typically want to create or use a read-only version of your child and return that instead:
Second, the locked, double-checked instantiation of the singleton is (much) better than nothing for making it thread-safe, but there are still some cases where this can fail to prevent double-instantiation. Microsoft instead recommends use of a static readonly field with instantiation defined:
Microsoft guarantees an implementation like this to be thread-safe in the Microsoft CLR; statics are instantiated just-in-time and the CLR has internal mechanisms to block other threads that need this reference while it's being created.