3

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:

  1. Am I taking the most reasonable approach in this case?
  2. Should I be worrying about const vs. readonly?
  3. 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.

6
  • 1
    "my question: ...(lots of text)... no question mark" - could you clarify a bit? I feel that a good portion of this post is only marginally related to your question and should not be included (such as mention of C++/CLI, differing opinions about singletons etc.) Commented Jun 6, 2012 at 20:35
  • In C# in general having private readonly int x = 1 is stronger than private int X { get; private set;} in that readonly can be set to some value ONLY on the same line as where they are declared, or initialized inside a constructor - so you are right that a property wrapper does not get you anything extra. For a better discussion you should post the code for the Singleton that you wrote because a singleton can fail in several ways, so details matter very much. It would also be much easier to see whether there exist non-singleton alternatives.
    – Job
    Commented Jun 6, 2012 at 20:36
  • 1
    Also, readonly modifier only guarantees that the reference pointer will not change, not that individual components of a list will not change. To guarantee that you would need to create a read-only list wrapper or something that extends IList<T>. As I said earlier, please try to paste some code. Sounds like you are talking to a database, so maybe an ORM library will help?
    – Job
    Commented Jun 6, 2012 at 20:39
  • 4
    Generally speaking, constant data is not considered a Singleton- just a constant.
    – DeadMG
    Commented Jun 6, 2012 at 20:42
  • @Job - thanks for the comments so far, and I have updated with a sample snippet of the code. There is more that it provides and does, but the snippet touches upon the points of my question. Fish - I tried to draw out my questions more clearly. Apologies for the previous lack of clarity.
    – user53019
    Commented Jun 6, 2012 at 21:09

1 Answer 1

2

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.

public class Foo
{
   public readonly List<int> Bar = new List<int>{1,2,3};
} 

...

var foo = new Foo();

//You may enumerate Bar and read its indices
foreach(int baz in foo.Bar) Console.WriteLine(baz.ToString());

var bob = foo.Bar[1];

//this is illegal because the reference is readonly
foo.Bar = new List<int>{4,5,6};

//but the properties of the immutable reference are still mutable, so this is fine:
foo.Bar.Clear();
foo.Bar.Add(4);
foo.Bar.Add(5);
foo.Bar.Add(6);

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:

public class Foo
{
   private readonly List<int> bar = new List<int>{1,2,3};

   public ReadOnlyCollection<int> Bar { get { return bar.AsReadOnly(); } }
} 

...

var foo = new Foo();

//You may still enumerate and index Bar
foreach(int baz in foo.Bar) Console.WriteLine(baz.ToString());

var bob = foo.Bar[1];

//this is illegal because there's no setter
foo.Bar = new List<int>{4,5,6};

//And these are also illegal because ReadOnlyCollection doesn't expose any such methods:
foo.Bar.Clear();
foo.Bar.Add(4);
foo.Bar.Add(5);
foo.Bar.Add(6);

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:

public sealed class MySingl
{
   public static readonly MySingl Instance = new MySingl();

   private MySingl() { ... }
}

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.

2
  • Thanks! Your example pointed out the concern that was sitting in the back of my head but couldn't quite identify.
    – user53019
    Commented Jun 19, 2012 at 16:35
  • In regards to the second issue, I recommend reading Jon Skeet's Singleton Article. He agrees with KiethS, though he makes his instance private and provides a property to access it. If you want laziness, I'd use Lazy<T>.
    – Brian
    Commented Jun 19, 2012 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.