Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
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.) –  Tamás Szelei Jun 6 '12 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 Jun 6 '12 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 Jun 6 '12 at 20:39
4  
Generally speaking, constant data is not considered a Singleton- just a constant. –  DeadMG Jun 6 '12 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. –  GlenH7 Jun 6 '12 at 21:09

1 Answer 1

up vote 1 down vote accepted

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.

share|improve this answer
    
Thanks! Your example pointed out the concern that was sitting in the back of my head but couldn't quite identify. –  GlenH7 Jun 19 '12 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 Jun 19 '12 at 17:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.