8
\$\begingroup\$

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());
\$\endgroup\$
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ludisposed Dec 19 '17 at 10:20
11
\$\begingroup\$

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).

\$\endgroup\$
  • 1
    \$\begingroup\$ Further to the comment about automagic projection: I would be inclined to separate that functionality out into it's own 'module' (if not using a pre-built one), so that you can retrieve a translator between T1 and T2 (e.g. Func<T1, T2> Mapping.Mapper<T1, T2>()). My experience with reflection has always been that providing a simple API to small well-defined tasks is pretty painless. Trying to juggle reflection and what is really just a "Select" is a recipe for misery. +1 \$\endgroup\$ – VisualMelon Dec 19 '17 at 7:14
  • \$\begingroup\$ Thanks for your reply, the reason I just do not use a select, is that in a real a life example there could be 100 plus in the classes and I dont want to be doing this type of thing a.name = b.name by x times. The reason I dont want to use AutoMapper is I want to improve my coding and knowledge \$\endgroup\$ – Jimbo Jones Dec 19 '17 at 8:48
  • \$\begingroup\$ The case could be made that, for real business purposes, using pre-built tools and libraries is an improved way to code. \$\endgroup\$ – Snowbody Dec 19 '17 at 14:16
  • 2
    \$\begingroup\$ @JimboJones In my opinion the best way to improve your coding knowledge is to first do what you did- implement it yourself- then go look at the library and see why theirs is (inevitably) much better :) \$\endgroup\$ – Ben Aaronson Dec 21 '17 at 15:13
10
\$\begingroup\$
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?

\$\endgroup\$
  • \$\begingroup\$ The reason I used a for loop was it appeared to be faster. \$\endgroup\$ – Jimbo Jones Dec 19 '17 at 8:55
  • 1
    \$\begingroup\$ @JimboJones "appeared" to be faster? Testing showed as much? And... is the faster worth the extra gymnastics? Is this a "critical" functioned used so much that ms saved is actually worth the more complicated code? \$\endgroup\$ – WernerCD Dec 19 '17 at 12:40
  • \$\begingroup\$ Yes when the execution was timed using a larger data set, it was fractionally faster using a for loop rather than a foreach, however I accept your point on readability. \$\endgroup\$ – Jimbo Jones Dec 19 '17 at 14:46
  • \$\begingroup\$ if you're concerned about speed, you shouldn't be using reflection at all! \$\endgroup\$ – Snowbody Dec 19 '17 at 14:55

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.