Is it bad to use reflection to simplify constructors, comparisons, etc?

Question

I hate having a bunch of "left/right" methods. Every time a property is added or removed, I have to fix up each method. And the code itself just looks ... wrong.

public Foo(Foo other)
{
    this.Bar = other.Bar;
    this.Baz = other.Baz;
    this.Lur = other.Lur;
    this.Qux = other.Qux;
    this.Xyzzy= other.Xyzzy;
}

Really this is just an unrolled loop that iterates through the properties, copying them between objects. So why not be honest about that fact? Reflection to the rescue!

public Foo(IFoo other)
{
    foreach (var property in typeof(IFoo).GetProperties())
    {
        property.SetValue(this, property.GetValue(other, null), null);
    }
}

I may just be trying to force a paradigm I learned from Lua onto C#, but this particular example doesn't seem too smelly to me. From here, I started to do some more complex things that were sensitive to the order of the fields. For example, rather than having a stack of virtually identical if statements to compose a string from the fields, I just iterate over them in the desired order:

public override string ToString()
{
    var toJoin = new List<string>();
    foreach (var property in tostringFields)
    {
        object value = property.GetValue(this, null);
        if (value != null)
            toJoin.Add(value.ToString());
    }
    return string.Join(" ", toJoin.ToArray());
}
private static readonly PropertyInfo[] tostringFields =
{
    typeof(IFoo).GetProperty("Bar"),
    typeof(IFoo).GetProperty("Baz"),
    typeof(IFoo).GetProperty("Lur"),
    typeof(IFoo).GetProperty("Qux"),
    typeof(IFoo).GetProperty("Xyzzy"),
};

So now I have the iterability I wanted, but I still have stacks of code mirroring each property I'm interested in (I'm also doing this for CompareTo, using a different set of properties in a different order). Worse than that is the loss of strong typing. This is really starting to smell.

Well what about using attributes on each property to define the order? I started down this road and indeed it worked well, but it just made the whole thing look bloated. It works great semantically, but I'm always wary of using advanced features just because they're "neat." Is using reflection in this way overkill? Is there some other solution to the left/right code problem I'm missing?

Answer 1

Using reflection in and of itself is not bad, but you will take a performance hit especially if you do it recursively.

I am not a fan of the hard coded copy constructors either because developers forget to update them when they add new properties to a class.

There are other ways of accomplishing what you want, including Marc Gravells Hyper Property Descriptor or if you want to learn some IL and OPCodes you can use System.Reflection.Emit or even Cecil from Mono.

Here's an example of using Hyper Property Descriptor that you can possibly tailor to your needs:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using Hyper.ComponentModel;
namespace Test {
    class Person {
        public int Id { get; set; }
        public string Name { get; set; }
    }
    class Program {
        static void Main() {
            HyperTypeDescriptionProvider.Add(typeof(Person));
            var properties = new Dictionary<string, object> { { "Id", 10 }, { "Name", "Fred Flintstone" } };
            Person person = new Person();
            DynamicUpdate(person, properties);
            Console.WriteLine("Id: {0}; Name: {1}", person.Id, person.Name);
            Console.ReadKey();
        }
        public static void DynamicUpdate<T>(T entity, Dictionary<string, object>  {
            foreach (PropertyDescriptor propertyDescriptor in TypeDescriptor.GetProperties(typeof(T)))
                if (properties.ContainsKey(propertyDescriptor.Name))
                    propertyDescriptor.SetValue(entity, properties[propertyDescriptor.Name]);
        }
    }
}

If you decide to carry on using reflection, you can reduce the performance hit by caching your calls to GetProperties() like so:

public Foo(IFoo other) {
    foreach (var property in MyCacheProvider.GetProperties<IFoo>())
        property.SetValue(this, property.GetValue(other, null), null);
}

Answer 2

IMHO, reflection is a very powerful feature of C#, but which is very likely to result in a bloated code, and which adds much to the learning curve of the code and reduces maintainability. You'll be more likely to commit mistakes (once basic refactoring may lead to errors), and more afraid to change the name of any property (if you happen to find a better name) or stuff like that.

I personally have a code with a similar problem, and I had the same idea of adding attributes to maintain order and etc. But my team (including me) thought it was better to lose some time changing the design not to need this. Perhaps this problem is caused by bad design (well, it was in my case, but I can't tell the same about yours).

Answer 3

I know there is already an answer to this, but I wanted to point out that there is a library that combines a few of the mitigation strategies for the performance impact that a few people have discussed.

The library is called AutoMapper and it maps from one object to another and does so by dynamically creating an IL assembly on the fly. This ensures that other than a first time hit, you get superior performance and your code would be much simpler:

public Foo(Foo other)
{
    Mapper.Map(other, this);
}

This tends to work great and has the added bonus of not being invented here, which I'm a fan of.

I did some performance testing and after the first hit of 20 ms (still pretty fast) it was about as close to 0 as you can get. Pretty impressive.

Hope this helps someone.

Answer 4

The basic problem is you are trying to use a statically typed language like a dynamically typed one.

There isn't really a need for anything fancy. If you want to be able to iterate the properties, you can use a Map<> as the backing store for all the properties in your class.

Coincidentally this is exactly how the VS project wizard implmements application settings for you. (see System.Configuration.ApplicationSettingsBase) Its also very 'lua-like'

   public bool ConfirmSync {
        get {
            return ((bool)(this["ConfirmSync"]));
        }
        set {
            this["ConfirmSync"] = value;
        }
    }