Copy a similar list to another

Question

My code works fine and I am just wondering is there a more efficient way to copy a similar list to another and ignore the properties which are not present.

private const BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.GetProperty | BindingFlags.SetProperty;

public static List<T> MergeListData<T>(List<object> collection)
{
    if (collection == null)
        throw new ArgumentNullException("Collection", "The Collection that you are copying to cannot be null");

    List<T> result = new List<T>();

    for (int x = 0; x < collection.Count; x++)
    {
        var target = (T)Activator.CreateInstance(typeof(T));
        var propInfos = collection[x].GetType().GetProperties(flags);
        for (int i = 0; i < propInfos.Length; i++)
        {
            try
            {
                PropertyInfo _propinfo = target.GetType().GetProperty(propInfos[i].Name, flags);

                if (_propinfo != null)
                    _propinfo.SetValue(target, Nullable.GetUnderlyingType(propInfos[i].PropertyType) ?? propInfos[i].GetValue(collection[x]));

            }
            catch (ArgumentException aex) { if (!string.IsNullOrEmpty(aex.Message)) continue; }
            catch (Exception ex) { if (!string.IsNullOrEmpty(ex.Message)) return default(List<T>); }
        }
        result.Add(target);
    }
    return result;
}

And the usage is:

public class Name1
{
    public int Number   { get; set; }
    public string First { get; set; }
    public string Last  { get; set; }
}

public class Name2
{
    public string First { get; set; }
    public string Last  { get; set; }
    public decimal Wages { get; set; }
    public bool Active { get; set; }
}

 var List1 = new List<Name1> {  new Name1{  First = "Billy", Number = 1, Last = "Harte"}, new Name1 { First = "Jimmy", Number = 1, Last = "Dunne" } };

 List<Name2> bs = MergeListData<Name2>(List1.Select(x => x as object).ToList());

Answer 1

Small things

Your code styling is generally good, but a few possible improvements:

• You can add a second generic type parameter to your method to avoid the need to cast to object. i.e. public static List<TIn,TOut> MergeListData<TOut>(List<TIn> collection)
• You can probably use a more general collection than List<object> for your input collection. If you use IEnumerable<object> (changing your for for a foreach, which you should probably do anyway), then you can avoid the cast without having to do the previous bullet point, since IEnumarable<T> is covariant
• Those exception handlers probably shouldn't be crammed onto a single line. If it were just the catch or the if, it'd be okay, but combining the two seems like too much
• _propinfo shouldn't have the underscore, this is usually used for class-level fields.

The general approach

More of an issue is the overall approach of using reflection here. Reflection has three major problems:

• It isn't safe. For example, here you could pass a generic type that doesn't have a default constructor and fail on the Activator.CreateInstance. This could be fixed with a generic type constraint, but that limits the useability of the method.
• It's hard to read and maintain. The previous bullet point is one potential issue I noticed. Are there others? Who knows, reflective code is really ugly, and verifying all the edge cases by eye is tough!
• It's slow. Whether this matters depends on context, but this method is pretty general-purpose, low-level, and operates on collections, which means that decent performance is at least desireable.

So, can we avoid it? Well, when you call the method, you already know what your two types are. Code with that knowledge should also know how convert between the two. So why not delete all that code and just write:

var bs = List1.Select(x => new Name2 { First = x.First, Last = x.Last }).ToList();

What you lose in automagicness you more than make up for in clarity, conciseness, verifiability, and performance. You can of course use a method somewhere instead of a lambda, to avoid spreading the mapping info all over the place.

If you really need this functionality (e.g. because there are lots of types, most of them can map to each other, and you don't feel like writing n^2 mapping methods)... well, that seems like an unusual situation, and I'd start by taking a step back and asking if this indicates a broader architectural problem. But if you've satisfied yourself that it's sensible, then I'd suggest using a library like AutoMapper rather than trying to roll this yourself. That way you don't have to concern yourself with how the magic works, and you get something much more mature and fully-featured (and, probably, performant).

Answer 2

for (int i = 0; i < propInfos.Length; i++)

Any particular reason why you're not using foreach (var prop in PropInfos) here?

catch (Exception ex) { if (!string.IsNullOrEmpty(ex.Message)) return default(List<T>); }

I consider this line to be dangerous. By using Pokemon exception handling ("gotta catch 'em all") and silently swallowing it, you prevent the caller from obtaining any information about how it failed. And what's the reason for the test? In what case would ex.Message be null or empty?