3
\$\begingroup\$

I wrote this factory class in order to pass parameter to Singlton class, is it a good design in term of design for multithreading environment?

public static class LoggingServiceFactory
{
    private static string _connectionstring;
    private static readonly Lazy<LoggingService> _INSTANCE = new Lazy<LoggingService>(() => new LoggingService(_connectionstring));

    public static ILoggingService GetService(string connectionString)
    {
        _connectionstring = connectionString;
        return _INSTANCE.Value;
    }

    private class LoggingService : ILoggingService
    {
        private string _connectionstring;
        internal LoggingService(string connectionString)
        {
            _connectionstring = connectionString;
        }

        public void LogMessage(string msg)
        {
            // do the logging work
        }

    }
}

public interface ILoggingService
{
    void LogMessage(string msg);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Think if caller passes TWO different connection strings. Second time she will get an instance built using first c.s. and it's absolutely unexpected... \$\endgroup\$ – Adriano Repetti Jan 26 '16 at 15:38
  • \$\begingroup\$ @AdrianoRepetti, OK, in this case, should it throw an exception? or to use a dependency injection and register the instance with parameter before anycode can see it. \$\endgroup\$ – Helic Jan 26 '16 at 15:52
  • \$\begingroup\$ DI unless connection string is somehow calculated at run-time. In that case you have to keep it as parameter but you may provide a Func<string> to retrieve connection when required. \$\endgroup\$ – Adriano Repetti Jan 26 '16 at 15:55
  • \$\begingroup\$ @AdrianoRepetti if DI is not available, how can you ensure users don't provide different Func<string> for the parameter? can you please provide an example? \$\endgroup\$ – Helic Jan 26 '16 at 16:00
  • \$\begingroup\$ They can provide multiple functions, of course, but you won't call them because instance has been already created. Of course in debug build you may always call it to check it didn't change. \$\endgroup\$ – Adriano Repetti Jan 26 '16 at 16:01
1
\$\begingroup\$

If connection string matching is a really necessary thing:

static class LoggingServiceFactory
{
    static readonly TaskCompletionSource<string> _cs = new TaskCompletionSource<string>();
    static ILoggingService _service;

    public static ILoggingService GetService(string connectionString)
    {
        if (_cs.TrySetResult(connectionString))
            _service = new LoggingService(connectionString);
        else
            if (_cs.Task.Result != connectionString)
                throw new InvalidOperationException("Connection string redefinition.");

        return _service;
    }
}
\$\endgroup\$
  • \$\begingroup\$ you used TaskCompletionSource to set the connectionstring, is it for thread safty purpose? \$\endgroup\$ – Helic Jan 27 '16 at 8:17
  • \$\begingroup\$ Yep, exactly. It is a tread safe atomic operation. Succeeds one time only. \$\endgroup\$ – Dmitry Nogin Jan 27 '16 at 8:19
  • \$\begingroup\$ Cool, it is a better way than using locks for such simple senario. \$\endgroup\$ – Helic Jan 27 '16 at 8:35
2
\$\begingroup\$

I would not personally put LoggingService implementation inside factory, as it reduces testability and usually needed to play some tricks with generics type parameters only.

To have a predictable behavior according to your design it might look like:

public static class LoggingServiceFactory
{
    static ConcurrentDictionary<string, ILoggingService> Services { get; } = 
        new ConcurrentDictionary<string, ILoggingService>();

    public static ILoggingService GetService(string connectionString) =>
        Services.GetOrAdd(connectionString, cs => new LoggingService(cs));
}

Anyway, it makes sense to think about delegating it to IoC container...

I don't think that it is a responsibility of this class to verify if connection string is the same. This interface delivers different perception.

\$\endgroup\$

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.