CODEWITHSUNDEEP
c#design-patternssingleton
John
John

Reputation: 1296

What's wrong with this singleton I created

I created a class which allows access to global access to a variable while only creating it once, essentially a singleton.

However, it doesn't match any of the 'correct' ways to implement a singleton. I assume that it's not mentioned because there is something 'wrong' with it but I can't see any problem with it other then the lack of lazy initialization.

Any thoughts?

static class DefaultFields
{
    private static readonly string IniPath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "defaultFields.ini");
    private static readonly IniConfigSource Ini = GetIni();               

    /// <summary>
    /// Creates a reference to the ini file on startup
    /// </summary>
    private static IniConfigSource GetIni()
    {
        // Create Ini File if it does not exist
        if (!File.Exists(IniPath))
        {
            using (FileStream stream = new FileStream(IniPath, FileMode.CreateNew))
            {
                var iniConfig = new IniConfigSource(stream);
                iniConfig.AddConfig("default");
                iniConfig.Save(IniPath);
            }
        }

        var source = new IniConfigSource(IniPath);
        return source;
    }

    public static IConfig Get()
    {
        return Ini.Configs["default"];
    }

    public static void Remove(string key)
    {
        Get().Remove(key);
        Ini.Save();
    }

    public static void Set(string key, string value)
    {
        Get().Set(key, value ?? "");
        Ini.Save();
    }
}

Upvotes: 4

Views: 573

Answers (8)

SqlRyan
SqlRyan

Reputation: 33914

The biggest problem I see is that you're not doing any SyncLock-ing around writes to your INI file - multiple threads that attempt to write values at the same time could end up with unpredictable results, like both making writes and only one persisting (or multiple threads attempting to write the file at once, resulting in an IO error).

I'd create a private "Locking" object of some kind and then wrap the writes to your file in a SyncLock to ensure that only one thread at a time has the ability to change values (or, at the very least, commit changes to the INI file).

Upvotes: 2

Mark Byers
Mark Byers

Reputation: 838376

All the methods on your class are static, so you are hiding the single instance from your users. With the singleton pattern the single instance is exposed via a public property usually called Instance (in other languages like Java it might be a method called getInstance or similar).

alt text

Your code is not wrong - it's just not the singleton pattern. If you wish to implement a singleton I would recommend Jon Skeet's article Implementing the Singleton Pattern in C#.

Upvotes: 2

Numenor
Numenor

Reputation: 1706

lazy initialization is very important for a singleton class. By declaring your class to be static you implement a static class, not a singleton class.

Upvotes: 0

Dr. Wily&#39;s Apprentice
Dr. Wily&#39;s Apprentice

Reputation: 10280

I am interested in answers to this question as well. In my opinion, there is a flood of singleton examples that use lazy instantiation, but I think you have to ask yourself if it's really necessary on a case by case basis.

Although this article pertains to Java, the concepts should still apply. This provides a number of examples for different singleton implementations. http://www.shaunabram.com/singleton-implementations/

I've also seen numerous references to the book "Effective Java", Item 71 - use lazy instantiation judiciously. Basically, don't do it unless you need to.

Upvotes: 1

J&#233;r&#233;mie Bertrand
J&#233;r&#233;mie Bertrand

Reputation: 3119

Why the readonly on the Ini field?

But if you want implement the singleton pattern, it's something like this:

static DefaultFields
{
    private readonly string IniPath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "defaultFields.ini");
    private readonly IniConfigSource Ini = GetIni();               

    private static DefaultFields _default;

    public static DefaultFields Default 
    { 
        get { if(this._default == null){ this._default = new DefaultFields(); } return this._default; } 
    }

    private DefaultFields()
    {

    }

    /// <summary>
    /// Creates a reference to the ini file on startup
    /// </summary>
    private IniConfigSource GetIni()
    {
        // Create Ini File if it does not exist
        if (!File.Exists(IniPath))
        {
            using (FileStream stream = new FileStream(IniPath, FileMode.CreateNew))
            {
                var iniConfig = new IniConfigSource(stream);
                iniConfig.AddConfig("default");
                iniConfig.Save(IniPath);
            }
        }

        var source = new IniConfigSource(IniPath);
        return source;
    }

    public IConfig Get()
    {
        return Ini.Configs["default"];
    }

    public void Remove(string key)
    {
        Get().Remove(key);
        Ini.Save();
    }

    public void Set(string key, string value)
    {
        Get().Set(key, value ?? "");
        Ini.Save();
    }
}

Upvotes: 0

Toby
Toby

Reputation: 7544

That's not really a singleton, it's a static class.

In many ways, static classes are similar to singleton's, true. But static classes can't implement interfaces, can't inherit functionality from a base class, and you can't carry a reference to them.

Upvotes: 0

Nuno Ramiro
Nuno Ramiro

Reputation: 1756

You are right about the singleton, its a class with a unique instance that provides global access.

It might seem like a static class but usually its implemented in a different way.

Also bear in mind that this pattern should be used with some precaution, as its really hard to refactor out a singleton once its deep in the code. Should be used primarily when you have hardware constraints or are implemented unique points of access to a factory. I would try to avoid it whenever possible.

An example of an implementation is as follows:

public class A
{
    /// <summary>
    /// Unique instance to access to object A
    /// </summary>
    public static readonly A Singleton = new A();

    /// <summary>
    /// private constructor so it can only be created internally.
    /// </summary>
    private A()
    {
    }

    /// <summary>
    /// Instance method B does B..
    /// </summary>
    public void B()
    {
    }
}

And could be used like

A.Singleton.B()

Hope that helps.

Upvotes: 4

Tristan
Tristan

Reputation: 3885

It doesn't follow the usual singleton patterns as your class is static and just controls access to static variables.

Where as a singleton is normally a static single instance of a class where the only static functions are that to create and access the singleton that stores variables as normal non static member variables.

Meaning the class could quite easily be changed or made to be instanced more then once but yours cannot

Upvotes: 5

Related Questions