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\$
7
  • 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\$ Jan 26, 2016 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, 2016 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\$ Jan 26, 2016 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, 2016 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\$ Jan 26, 2016 at 16:01

2 Answers 2

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\$
3
  • \$\begingroup\$ you used TaskCompletionSource to set the connectionstring, is it for thread safty purpose? \$\endgroup\$
    – Helic
    Jan 27, 2016 at 8:17
  • \$\begingroup\$ Yep, exactly. It is a tread safe atomic operation. Succeeds one time only. \$\endgroup\$ Jan 27, 2016 at 8:19
  • \$\begingroup\$ Cool, it is a better way than using locks for such simple senario. \$\endgroup\$
    – Helic
    Jan 27, 2016 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 and acknowledge you have read our privacy policy.

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