6
\$\begingroup\$

I am looking to implement a method that is capable of merging 2 dictionaries using generics. I've seen several great answers on SO already, but none handle the case of nested dictionaries. As in, what if the value of the dictionaries is another dictionary.

In order to tackle this, I created the following method:

using System;
using System.Collections.Generic;
using System.Reflection;

namespace CollectionHelpers
{
   /// <summary>
   /// This class contains helpful methods for manipulating collections.
   /// Utilizes .NET 4.0 features.
   /// </summary>
   public class DictionaryHelper
   {
       #region Dictionary
       /// <summary>
       /// Unionise two dictionaries of generic types.
       /// Duplicates take their value from the leftmost dictionary.
       /// </summary>
       /// <typeparam name="T1">Generic key type</typeparam>
       /// <typeparam name="T2">Generic value type</typeparam>
       /// <param name="D1">Dictionary 1</param>
       /// <param name="D2">Dictionary 2</param>
       /// <returns>The combined dictionaries.</returns>
       public static Dictionary<T1, T2> UnionDictionaries<T1, T2>(Dictionary<T1, T2> D1, Dictionary<T1, T2> D2)
       {
           Dictionary<T1, T2> rd = new Dictionary<T1, T2>(D1);
           foreach (var key in D2.Keys)
           {
               if (!rd.ContainsKey(key))
                   rd.Add(key, D2[key]);
               else if(rd[key].GetType().IsGenericType)
               {
                   if (rd[key].GetType().GetGenericTypeDefinition() == typeof(Dictionary<,>))
                   {
                       var mBase = MethodBase.GetCurrentMethod();
                       MethodInfo info = mBase is MethodInfo ? (MethodInfo)mBase : typeof(DictionaryHelper).GetMethod("UnionDictionaries", BindingFlags.Public | BindingFlags.Static);
                       var genericMethod = info.MakeGenericMethod(rd[key].GetType().GetGenericArguments()[0], rd[key].GetType().GetGenericArguments()[1]);
                       var invocationResult = genericMethod.Invoke(null, new object[] { rd[key], D2[key] });
                       rd[key] = (T2)invocationResult;
                   }
               }
           }
           return rd;
       }
       #endregion
   }
}

The code works for the samples I've thrown at it so far. Obviously I can't test all cases. This is my attempt to improve on a previous design which used IDictionary, but that function is not safe since it doesn't check the actual types of the dictionaries it handles.

    /// <summary>
    /// Merge two IDictionaries.
    /// If the values of two matching keys are Ilists then they will be merged without duplicates as well.
    /// If not then the value of the leftmost targetDictionary will be maintained.
    /// </summary>
    /// <param name="targetDictionary">The dictionary to merge all values in to.</param>
    /// <param name="sourceDictionary">The dictionary to merge values from.</param>
    /// <returns></returns>
    public static IDictionary MergeCollections(IDictionary targetDictionary, IDictionary sourceDictionary)
    {
        foreach (var key in sourceDictionary.Keys)
        {
            if (!targetDictionary.Contains(key))
                targetDictionary.Add(key, sourceDictionary[key]);
            else
            {
                if(targetDictionary[key] is IDictionary)
                    targetDictionary[key] = MergeCollections((IDictionary)targetDictionary[key], (IDictionary)sourceDictionary[key]);
                else if (targetDictionary[key] is IList)
                    targetDictionary[key] = MergeCollections((IList)targetDictionary[key], (IList)sourceDictionary[key]);
            }
        }
        return targetDictionary;
    }

The overloaded MergeCollections method with IList parameters doesn't matter right now.

Which one of these would you prefer and why? Suggestions for improvements and other ways of tackling this problem are welcome.

\$\endgroup\$
5
\$\begingroup\$
  1. While using K and V as generic type parameters is an improvement, I would not stop there, and use TKey and TValue instead.
  2. I believe the .Net naming convention recommends using camelCase for public method parameters (which you use in your second example, but not in first). Also targetDictionary is way better name then D1 in general, camel case or not.
  3. I think this is the case where extension method would make more sense, than regular static method.
  4. is operator performs a cast. If you need the result of that cast later on, then you should probably use as instead, and check the result for null. Otherwise you perform the cast twice. I think it is unlikely to become some kind of bottleneck performance-wise, but it's still worth mentioning.
  5. In the end, reflection being the obvious downside of the first approach, I would probably go with the second one. Just wrap it with generic method, and you are good, IMHO:

    public static Dictionary<TKey, TValue> MergeCollections<TKey, TValue>(Dictionary<TKey, TValue> targetDictionary, Dictionary<TKey, TValue> sourceDictionary)
    {
        return (Dictionary<TKey, TValue>)MergeCollections((IDictionary)targetDictionary, (IDictionary)sourceDictionary);
    }
    
    private static IDictionary MergeCollections(IDictionary targetDictionary, IDictionary sourceDictionary)
    {
        ....
    }
    
\$\endgroup\$
  • \$\begingroup\$ Thanks for the useful tips. I might run some performance tests on both methods later to see if there is really any significant difference. \$\endgroup\$ – Laurens Vergote Jun 25 '15 at 7:02
3
\$\begingroup\$

I think it's important to remember that T is not always the best name for generic types. This is definitely important to remember when you start numbering them...

public static Dictionary<T1, T2> UnionDictionaries<T1, T2>(Dictionary<T1, T2> D1, Dictionary<T1, T2> D2)

To make matters worse, you decided to use parameter names for the dictionaries that almost identically match these types. It took me far longer than it should have to grok what is really a pretty simple signature. Being T1 represents a Key's type and T2 represents a Value's type, I would recommend the rather idiomatic K and V over the generic T name. (No pun intended, I swear.)

public static Dictionary<K, V> UnionDictionaries<K, V>(Dictionary<K, V> dict1, Dictionary<K, V> dict2)

This here is overly verbose. It's obvious what the type is from the right hand side of the assignment.

Dictionary<T1, T2> rd = new Dictionary<T1, T2>(D1);

Use the var keyword and give rd a reasonable name.

var result = new Dictionary<K, V>(dict1);

You should really be using braces for any conditionals.

           if (!rd.ContainsKey(key))
               rd.Add(key, D2[key]);
           if (!result.ContainsKey(key))
           {
               result.Add(key, dict2[key]);
           }

You have an opportunity to reduce the nesting here.

       else if(rd[key].GetType().IsGenericType)
       {
           if (rd[key].GetType().GetGenericTypeDefinition() == typeof(Dictionary<,>))
           if (!result.ContainsKey(key))
           {
               result.Add(key, dict2[key]);
           }
           else if(result[key].GetType().IsGenericType && result[key].GetType().GetGenericTypeDefinition() == typeof(Dictionary<,>))
           {
               var mBase = MethodBase.GetCurrentMethod();
               MethodInfo info = mBase is MethodInfo ? (MethodInfo)mBase : typeof(DictionaryHelper).GetMethod("UnionDictionaries", BindingFlags.Public | BindingFlags.Static);
               var genericMethod = info.MakeGenericMethod(result[key].GetType().GetGenericArguments()[0], result[key].GetType().GetGenericArguments()[1]);
               var invocationResult = genericMethod.Invoke(null, new object[] { result[key], dict2[key] });
               result[key] = (V)invocationResult;
           }

But that's pretty verbose, so go ahead and extract a private method to do that boolean check.

   private IsGenericDictionary<V>(V value)
   {
       var type = value.GetType();
       return type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Dictionary<,>))
   }

Current state of the code:

   public static Dictionary<K, V> UnionDictionaries<K, V>(Dictionary<K, V> dict1, Dictionary<K, V> dict2)
   {
       var result = new Dictionary<K, V>(dict1);
       foreach (var key in dict2.Keys)
       {
           var value = result[key];

           if (!result.ContainsKey(key))
           {
               result.Add(key, dict2[key]);
           }
           else if(IsGenericDictionary(value))
           {
               var mBase = MethodBase.GetCurrentMethod();
               MethodInfo info = mBase is MethodInfo ? (MethodInfo)mBase : typeof(DictionaryHelper).GetMethod("UnionDictionaries", BindingFlags.Public | BindingFlags.Static);
               var genericMethod = info.MakeGenericMethod(result[key].GetType().GetGenericArguments()[0], result[key].GetType().GetGenericArguments()[1]);
               var invocationResult = genericMethod.Invoke(null, new object[] { result[key], dict2[key] });
               result[key] = (V)invocationResult;
           }
       }
       return result;
   }
\$\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.