2
\$\begingroup\$
class Connection 
{
    private string param1;
    private string param2;
    private static readonly ConcurrentDictionary<Tuple<string, string>, Connection> 
        connections = new ConcurrentDictionary<Tuple<string, string>, Connection>();
    private Connection()
    {
    //Prevent instantiation
    }

    private Connection(string param1, string param2)
    {
        this.param1 = param1;
        this.param2 = param2;
    }

    public static Connection getInstance(string param1, string param2)
    { 
        Connection conn = activeConnections.GetOrAdd(new Tuple<string, string> 
            param1,param2), new Connection (param1, param2));
        return conn;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you saying that if two threads ask for a connection with the same parameters, they should get the same Connection? That means the actual code of Connection (the one that uses the parameters and that you didn't show) has to be thread-safe too. \$\endgroup\$ – svick Jun 18 '13 at 13:21
  • \$\begingroup\$ Also, it's okay to create duplicate Connection objects as long as nobody ever sees them, right? (Because that's what you do.) \$\endgroup\$ – svick Jun 18 '13 at 13:25
  • \$\begingroup\$ @svick Ok so here is what I intended to do: 1. If two threads ask for a connection with the same parameters, only one connection object has to be created & used by them. 2. Never create two connection objects with the same parameters. If one exists, use it. Else create a new one. Is this what I am doing or not? If this is a bad idea, do elaborate. \$\endgroup\$ – rtindru Jun 19 '13 at 11:56
1
\$\begingroup\$

Never create two connection objects with the same parameters. If one exists, use it.

If you really need to guarantee this, then I think you will need to use locking instead of ConcurrentDictionary.

If it's okay to create duplicate Connections (that will never be used) in rare circumstances, then you can use an overload of GetOrAdd() that takes a lambda that creates the Connection:

return activeConnections.GetOrAdd(
    Tuple.Create(param1,param2), _ => new Connection (param1, param2));

With your current code, every time you call getInstance(), a new Connection is created and then most of the time thrown away.

\$\endgroup\$
  • \$\begingroup\$ Ok, I checked if that happens in a crude way, I used a class member - Guid which instantiated every time I created a class. When I created two objects with same params, the Guids were the same >> SO I assumed they were referencing the same (single) object. If 2 objects are being created, then they must have different GUID right? Can you explain that? \$\endgroup\$ – rtindru Jun 19 '13 at 13:42
  • \$\begingroup\$ @rtindru Two objects are being created, but both calls to GetOrAdd() will return the same object. Basically, call 2 creates new Connection, realizes the dictionary already contains one, so it throws the one it created away and returns the one from the dictionary. \$\endgroup\$ – svick Jun 19 '13 at 13:59
  • \$\begingroup\$ Oh, that seems like a waste of time and effort. Are you sure about this? THis is what the msdn website says: Adds a key/value pair to the ConcurrentDictionary<TKey,Value> if the key does not already exist. [link]msdn.microsoft.com/en-us/library/ee378676.aspx \$\endgroup\$ – rtindru Jun 20 '13 at 5:20
  • \$\begingroup\$ @rtindru ConcurrentDictionary doesn't change how C# works. And method parameters are always evaluated before the method is invoked. \$\endgroup\$ – svick Jun 20 '13 at 6:47
  • \$\begingroup\$ hmm... Alright, then what is the alternative that you suggest. Because clearly this is a vain effort. \$\endgroup\$ – rtindru Jun 20 '13 at 7:39
1
\$\begingroup\$

yes. But it's ugly. You won't remember what those parameters means when you come back in a couple of months and have to maintain the code.

I really hate convenience classes like Tuple and Action<T> (the latter is OK some times but is usually abused).

It's much better to create small classes which actually describes what the values represent.

\$\endgroup\$
  • \$\begingroup\$ Ok so create a class called ParamDetails with the values and use that as the Key? Keys do not have to be primitive datatypes? \$\endgroup\$ – rtindru Jun 18 '13 at 11:49
  • \$\begingroup\$ Tuple is not a primitive either. But you have to implement GetHashCode(). \$\endgroup\$ – jgauffin Jun 18 '13 at 11:53
  • \$\begingroup\$ In this specific case, I disagree. I think Tuple as an implementation detail is okay, when it's clear from the surrounding code that Item1 and Item2 actually mean param1 and param2 (assuming those actually have meaningful names in real code). \$\endgroup\$ – svick Jun 18 '13 at 13:26
1
\$\begingroup\$

If you are on V4 or later, Lazy<T> might help:

private static readonly ConcurrentDictionary<Tuple<string, string>, Lazy<Connection>> 
    connections = new ConcurrentDictionary<Tuple<string, string>, Lazy<Connection>>();

public static Connection getInstance(string param1, string param2)
{ 
    var connLazy = activeConnections.GetOrAdd(
        Tuple.Create(param1,param2), 
        new Lazy<Connection>(
            () => new Connection (param1, param2),
            LazyThreadSafetyMode.ExecutionAndPublication
        );
    return conn.Value;
}

GetOrAdd ensures that every threads will get same object, and extra Lazy<Connection> will be discarded. LazyThreadSafetyMode.ExecutionAndPublication mode ensures that the instance will be initiated only once and no more.

\$\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.