Preamble: it's a self-assigned and pure syntetic task to learn (and remember what I already knew) C# threads and synchronization and data structures.

Let's say I have a dictionary <string, string> that represents a path (http) to a file by some key, ie:

foo => http://domain.tld/file1
bar => http://domain2.tld/file2


And I'd like to implement a class that will implement an interface with 2 methods:

String Rand();
String Get(String key);


The first method would pick the file randomly from all the available, and the Get would return a particular file, or to be precise - local path to a downloaded file.

The class should be thread-safe, so that if several threads request the same key with Get() or the Rand() picks the same item - then only one thread should actually download a file to a local drive, or the path should be retrieved immediately if a file has already been downloaded.

My proof of concept solution is:

class Downloader
{
private IDictionary<string, string> _storage = new Dictionary<string, string>();

{
_map = map ?? new Dictionary<string, string>();
}

public string Get(string key)
{
if (_storage.ContainsKey(key))
{
return _storage[key];
}

{
}

{
return Get(key);
}

}

{
string path;

if (!_map.TryGetValue(key, out path))
{
throw new ArgumentException("The specified key wasn't found");
}

{
Console.WriteLine("Started retrieving {0}", k.ToString());
Console.WriteLine("Finished retrieving {0}", k.ToString());
return k.ToString() + " local path";
}, path);
}
}


The copy is at http://pastebin.com/LJnK7FB6

How could I make it better? Any particular problems with the code?

In the comments @I4V pointed out to some problems at https://stackoverflow.com/questions/16458937/synchronization-of-remote-files-download#comment23617420_16460673 but seems like I'm missing the point

If you change _retrieveFile to return a Task that's already started, you could simplify the whole Get() into just:
return _progress.GetOrAdd(key, _retrieveFile).Result;

Also, it seems that Downloader with empty map doesn't make sense, so you should check for that in your constructor, instead of using ??.
• That's a nice point actually, but shouldn't it be _retrieveFile()? – zerkms May 9 '13 at 21:37
• @zerkms No, there is an overload that accepts Func. Though I assumed that it's guaranteed that the delegate won't be executed more than once, which, according to the documentation is not true. So my code might not be good enough for you. – svick May 9 '13 at 21:43