Improve this reflection bashing code

Question

I have implemented an IDataContractSurrogate to enable serialization of ImmutableList<T> (from the Microsoft Immutable Collections BCL) using the DataContractSerialization framework.

The code is a bit wild using reflection and compiling of delegates. Are there any subtle improvements or obvious over engineering problems here?

This is a follow-up from a Stack Overflow question, but further improvements feel like code review rather than specific questions.

TESTCASE

using FluentAssertions;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using System.Runtime.Serialization;
using System.Text;
using System.Xml;
using Xunit;

namespace ReactiveUI.Ext.Spec
{
    [DataContract(Name="Node", Namespace="http://foo.com/")]
    class Node
    {
        [DataMember()]
        public string Name;
    }

    [DataContract(Name="Fixture", Namespace="http://foo.com/")]
    class FixtureType
    {
        [DataMember()]
        public ImmutableList<Node> Nodes;

        public FixtureType(){
            Nodes = ImmutableList<Node>.Empty.AddRange( new []
            { new Node(){Name="A"}
            , new Node(){Name="B"}
            , new Node(){Name="C"}
            });
        }
    }

    public class ImmutableSurrogateSpec
    {  
        public static string ToXML(object obj)
            {
                var settings = new XmlWriterSettings { Indent = true };

                using (MemoryStream memoryStream = new MemoryStream())
                using (StreamReader reader = new StreamReader(memoryStream))
                using (XmlWriter writer = XmlWriter.Create(memoryStream, settings))
                {
                    DataContractSerializer serializer =
                      new DataContractSerializer
                          ( obj.GetType()
                          , new DataContractSerializerSettings() { DataContractSurrogate = new ImmutableSurrogateSerializer() }
                          );
                    serializer.WriteObject(writer, obj);
                    writer.Flush();
                    memoryStream.Position = 0;
                    return reader.ReadToEnd();
                }
            }

        public static T Load<T>(Stream data)
        {
            DataContractSerializer ser = new DataContractSerializer
                  ( typeof(T)
                  , new DataContractSerializerSettings() { DataContractSurrogate = new ImmutableSurrogateSerializer() }
                  );
            return (T)ser.ReadObject(data);
        }

        public static T Load<T>(string data)
        {
            using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(data)))
            {
                return Load<T>(stream);
            }
        }

        [Fact]
        public void ShouldWork()
        {
            var o = new FixtureType();

            var s = ToXML(o);

            var oo = Load<FixtureType>(s);

            oo.Nodes.Count().Should().Be(3);
            var names = oo.Nodes.Select(n => n.Name).ToList();
            names.ShouldAllBeEquivalentTo(new[]{"A", "B", "C"});                
        }
    }
}

IMPLEMENTATION

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.Serialization;

namespace ReactiveUI.Ext
{
    class ImmutableListListConverter<T>
    {
        public static ImmutableList<T> ToImmutable( List<T> list )
        {
            return ImmutableList<T>.Empty.AddRange(list);
        }

        public static List<T> ToList(ImmutableList<T> list){
            return list.ToList();
        }

        public static object ToImmutable( object list )
        {
            return ToImmutable(( List<T> ) list);
        }

        public static object ToList(object list){
            return ToList(( ImmutableList<T> ) list);
        }
    }

    static class ImmutableListListConverter {
        static ConcurrentDictionary<Tuple<string, Type>, Func<object,object>> _MethodCache 
            = new ConcurrentDictionary<Tuple<string, Type>, Func<object,object>>();

        public static Func<object,object> CreateMethod( string name, Type genericType )
        {
            var key = Tuple.Create(name, genericType);
            if ( !_MethodCache.ContainsKey(key) )
            {
                _MethodCache[key] = typeof(ImmutableListListConverter<>)
                    .MakeGenericType(new []{genericType})
                    .GetMethod(name, new []{typeof(object)})
                    .MakeLambda();
            }
            return _MethodCache[key];
        }
        public static Func<object,object> ToImmutableMethod( Type targetType )
        {
            return ImmutableListListConverter.CreateMethod("ToImmutable", targetType.GenericTypeArguments[0]);
        }

        public static Func<object,object> ToListMethod( Type targetType )
        {
            return ImmutableListListConverter.CreateMethod("ToList", targetType.GenericTypeArguments[0]);
        }

        private static Func<object,object> MakeLambda(this MethodInfo method )
        {
            return (Func<object,object>) method.CreateDelegate(Expression.GetDelegateType(
            (from parameter in method.GetParameters() select parameter.ParameterType)
            .Concat(new[] { method.ReturnType })
            .ToArray()));
        }   
    }

    public class ImmutableSurrogateSerializer : IDataContractSurrogate
    {
        static ConcurrentDictionary<Type, Type> _TypeCache = new ConcurrentDictionary<Type, Type>();

        public Type GetDataContractType( Type targetType )
        {
            if ( _TypeCache.ContainsKey(targetType) )
            {
                return _TypeCache[targetType];
            }

            if(targetType.IsGenericType && targetType.GetGenericTypeDefinition() == typeof(ImmutableList<>)) 
            {
                return _TypeCache[targetType] 
                    = typeof(List<>).MakeGenericType(targetType.GetGenericArguments());
            }
            else
            {
                return targetType;
            }
        }

        public object GetDeserializedObject( object obj, Type targetType )
        {
            if ( _TypeCache.ContainsKey(targetType) )
            {
               return ImmutableListListConverter.ToImmutableMethod(targetType)(obj);
            }
            return obj;
        }

        public object GetObjectToSerialize( object obj, Type targetType )
        {
            if ( targetType.IsGenericType && targetType.GetGenericTypeDefinition() == typeof(ImmutableList<>) )
            {
               return ImmutableListListConverter.ToListMethod(targetType)(obj);
            }
            return obj;
        }

        public object GetCustomDataToExport( Type clrType, Type dataContractType )
        {
            throw new NotImplementedException();
        }

        public object GetCustomDataToExport( System.Reflection.MemberInfo memberInfo, Type dataContractType )
        {
            throw new NotImplementedException();
        }

        public void GetKnownCustomDataTypes( System.Collections.ObjectModel.Collection<Type> customDataTypes )
        {
            throw new NotImplementedException();
        }

        public Type GetReferencedTypeOnImport( string typeName, string typeNamespace, object customData )
        {
            throw new NotImplementedException();
        }

        public System.CodeDom.CodeTypeDeclaration ProcessImportedType( System.CodeDom.CodeTypeDeclaration typeDeclaration, System.CodeDom.CodeCompileUnit compileUnit )
        {
            throw new NotImplementedException();
        }

        public ImmutableSurrogateSerializer() { }
    }
}

Answer 1

This actually looks pretty nice and I am bookmarking it. :)

Two tiny details, which are probably a waste of time:

1. I would personally (probably) make the ImmutableListListConverter<T> a private class inside of the ImmutableListListConverter, since it's an implementation detail of the latter class.

2. I usually try to avoid looking up a hash table multiple times, even though the performance difference is usually negligible. In your case, ImmutableSurrogateSerializer.GetDataContractType could probably avoid looking at the dictionary most of the time by doing the if guard at the beginning and then doing the lookup/insertion in a single step. Also, the indexer right after ContainsKey results in that unnecessary second lookup. Note that this is what I find simpler to read, not because the performance difference is anything but negligible:

   public Type GetDataContractType(Type t)
   {
      // if this is not an immutable list, we can just skip the rest immediately 
      if (!t.IsGenericType || t.GetGenericTypeDefinition() != typeof(ImmutableList<>))
      { 
          return type;
      }
   
      // get the result through the cache
      return _TypeCache.GetOrAdd(t, t => typeof(List<>).MakeGenericType(t.GetGenericArguments()));
   }
   

But as I've written, apart from that, it's a really nice approach. Proper de/serialization of immutable types is something I've been looking for since .NET 2.0 and the plain old XmlSerializer.