4
\$\begingroup\$

As I understand creating lots of resource managers in C# may be a bad idea so I thought that the best thing to do would be to create a singleton for this.

However, I'm not 100% if this is a good idea, or if this is a good way to implement it.

The main thing about the implementation I am unsure about is making the "Instance" private and creating number of static functions to expose the functionality. This means I can call Local.GetString instead of Local.Instance.GetString which is less clunky. I have not seen any singleton implementation which does this so I wonder if there is some problem with it.

I am also using locking but I don't see how to get around that so that is probably fine.

public class Locale
{
    // using System.Lazy<T> -- see http://msdn.microsoft.com/en-us/library/dd642331.aspx
    static readonly Lazy<Locale> lazy = new Lazy<Locale>(() => new Locale(), true);
    static readonly object _locker = new object();

    Dictionary<Type, ResourceManager> _managers;

    public Locale()
    {
        _managers = new Dictionary<Type, ResourceManager>();
    }

    static Locale Instance { get { return lazy.Value; } }

    public static ResourceManager GetManager(Type resourceType)
    {
        return Instance.GetResourceManager(resourceType);
    }

    public static string GetString(Type resourceType, string name)
    {
        var manager = Instance.GetResourceManager(resourceType);
        return manager.GetString(name);
    }

    public static string GetString(Type resourceType, string name, CultureInfo culture)
    {
        var manager = Instance.GetResourceManager(resourceType);
        return manager.GetString(name, culture);
    }

    public static object GetObject(Type resourceType, string name)
    {
        var manager = Instance.GetResourceManager(resourceType);
        return manager.GetObject(name);
    }

    public static object GetObject(Type resourceType, string name, CultureInfo culture)
    {
        var manager = Instance.GetResourceManager(resourceType);
        return manager.GetObject(name, culture);
    }

    ResourceManager GetResourceManager(Type resourceType)
    {
        lock (_locker)
        {
            ResourceManager manager;
            if (!_managers.TryGetValue(resourceType, out manager))
            {
                manager = new ResourceManager(resourceType);
                _managers.Add(resourceType, manager);
            }
            return manager;
        }
    }
}

I am using it like so

public class LocalizedDescriptionAttribute : DescriptionAttribute    
{
    readonly Type _resourceType;
    readonly string _resourceName;
    readonly CultureInfo _culture;

    public LocalizedDescriptionAttribute(Type resourceType, string resourceName)
        : this(resourceType, resourceName, null)
    {
    }

    public LocalizedDescriptionAttribute(Type resourceType, string resourceName, CultureInfo cultureInfo)
        : base()
    {
        _resourceType = resourceType;
        _resourceName = resourceName;
        _culture = cultureInfo;
    }

    public override string Description
    {
        get
        {
            if (_culture == null)
                return Locale.GetString(_resourceType, _resourceName);
            return
                Locale.GetString(_resourceType, _resourceName, _culture);
        }
    }      
}

I have of course read Jon Skeet's article on singletons.

\$\endgroup\$
  • \$\begingroup\$ I think this may be an extension of the same mistake that results in the use of ServiceLocators, in a slightly less dangerous manner. Both are a mess to try to test. \$\endgroup\$ – Magus Jan 23 '14 at 23:23
3
\$\begingroup\$

The main thing about the implementation I am unsure about is making the "Instance" private and creating number of static functions to expose the functionality. This means I can call Local.GetString instead of Local.Instance.GetString which is less clunky. I have not seen any singleton implementation which does this so I wonder if there is some problem with it.

I don't think there's a problem with that. Your static methods are only doing things which otherwise the calling code would do.

I am also using locking but I don't see how to get around that so that is probably fine.

If your code is single-threaded then you don't need locking at all.

If your code is multi-threaded, perhaps you can construct the singleton before the multi-threading starts: for example by calling a static Locale.CreateInstance method in your program's Main (or from Application_Start if your code is ASP.NET).

If your code is multi-threaded then the non-static Locale methods must also be thread-safe: including your Locale._managers dictionary accessors, and your ResourceManager methods. ResourceManager is already documented as a thread-safe class.

An alternative to the explicit lock you implemented is to use the ConcurrentDictionary class instead of Dictionary, specifically its GetOrAdd method, something like:

ResourceManager GetResourceManager(Type resourceType)
{
    Func<Type, ResourceManager> valueFactory =
        resourceType => new ResourceManager(resourceType);
    return managers.GetOrAdd(resourceType, valueFactory);
}

Stylistically, some people recommend auto-declaring variables types as var for example ...

    var valueFactory = resourceType => new ResourceManager(resourceType);

... or simply ...

ResourceManager GetResourceManager(Type resourceType)
{
    return managers.GetOrAdd(resourceType, resourceType => new ResourceManager(resourceType));
}
\$\endgroup\$
  • \$\begingroup\$ Oh, I didn't know about the ConcurrentDictionary class. I'll change my code to use that. I don't really understand your other comments about locking. Are you saying there is something else I am missing, that it isn't thread safe? The locking is to avoid creating multiple resource managers with the same "type". The only point I am currently accessing the _managers field is under that lock. And the Lazy<> code (thread-safe flag is set to true) should take care of making the singleton thread-safe. I don't see what having a Locale.CreateInstance would do or solve. \$\endgroup\$ – John Jan 21 '14 at 14:15
  • \$\begingroup\$ Locale.CreateInstance at program startup would guarantee it's created before it's used, and mean that you don't need to use Lazy. Perhaps you're trying to avoid locks altogether, and Lazy presumably includes a lock in its implementation. \$\endgroup\$ – ChrisW Jan 21 '14 at 14:21
  • \$\begingroup\$ It looks thread-safe as-is. I'm just trying to reduce the number of explicit locks and lines of code (use ConcurrentDictionary instead of Dictionary), and the number of implicit locks (perhaps create on startup instead of using Lazy). \$\endgroup\$ – ChrisW Jan 21 '14 at 14:23
  • \$\begingroup\$ You said, "I am also using locking but I don't see how to get around that", so I was showing ways to get around that. I was also trying to warn that if ResourceManager weren't already thread-safe, then the code wouldn't be thread-safe. \$\endgroup\$ – ChrisW Jan 21 '14 at 14:31
  • \$\begingroup\$ Oh I see. Well think I'm willing to accept the cost of a Lazy thread-safety. I think it's far more convenient then having to remember to call Locale.CreateInstance (this is for a rather complex system) \$\endgroup\$ – John Jan 21 '14 at 14:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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