Optimal way to update property of Generic type for this scenario

Question

I am adding some localization logic that I want to use application wide and instead of creating it multiple times I have decided to create a Generic method for this scenario and it works, but my Generics knowledge is limited, and it just doesn't feel optimal...

Method:

public List<T> Localize<T>(List<T> inputList, string propertyName)
{
    var filteredList = new List<T>();

    foreach (var item in inputList)
    {
        var clone = Mapper.Map<T, T>(item);
        // "TranslationKey" is the fixed/same column name for every entity/table.
        var translationKey = item.GetType().GetProperty("TranslationKey").GetValue(clone).ToString();

        if (!string.IsNullOrEmpty(translationKey))
        {
            var prop = clone.GetType().GetProperty(propertyName);
            prop.SetValue(clone, TranslateAsync(translationKey).Result);
        }
        filteredList.Add(clone);
    }
    return filteredList;
}

Then I would call the method like so:

var items = Localize(itemsList, "Description"); // To localize "Description" prop
var people = Localize(peopleList, "Name"); // To localize "Name" prop of People list...

So my two main questions are:

1. Can I optimize my methods implementation?
2. Is there a way I could not hard code property names when calling the method or is there a better/faster LINQ way or something I am missing?

Answer 1

Adding this answer (arrived from other answers) as yet another alternative for future, this way I can expose Localize to Singular and Multiple objects:

public List<T> Localize<T>(List<T> inputList, Action<T, string> setter) where T : ITranslatable
{
    return inputList.Select(a => Localize(a, setter)).ToList();
}

public T Localize<T>(T input, Action<T, string> setter) where T : ITranslatable
{
    var clone = Mapper.Map<T, T>(input);
    var translationKey = clone.TranslationKey;
    if (!string.IsNullOrEmpty(translationKey))
    {
        setter(clone, TranslateAsync(translationKey).Result);
    }
    return clone;
}   

Interface:

public interface ITranslatable
{
    string TranslationKey{ get; }
}

Call like this:

Localize(items, (i, val) => i.Description= val); // etc.

Answer 2

Since reflection is used for getters/setters, which can probably be avoided, there is indeed room for optimization and more importantly making the code safer in case objects are changed in the future.

The first reflection item.GetType().GetProperty("TranslationKey").GetValue(clone) implies that type T always has a property TranslationKey. The generic constraints can enforce that, as long as all objects implement the proper interface (or share the same base class) For example, the Item class and People class both implement something like interface ITranslationEntity{string TranslationKey{get;}} and if the method signature contains where T:ITranslationEntity, the property can be accessed directly

The setter could be done with the same interface implementation, e.g. have a SetTranslatedValue(string) on the interface, or the setter can be implemented as a lambda public List<T> Localize<T>(List<T> inputList, Action<T,string> setter) Used the latter as an example here, but having it in the interface would personally seem the better choice.

A small extra change which could make the call easier, but depends on whether you can place this method in a static class, is to make the method an extension method.

Perhaps the most important part is that the async call is executed synchronously in the loop (by accessing the Result ). Perhaps the implementation isn't truly asynchronous, but if it is, the major optimization is to combine the async calls and await the set of Tasks (and set the result inside a ContinueWith

End result:

static class SomeClass
{

    public static List<T> Localize<T>(this List<T> inputList, Action<T,string> setter)
        where T:ITranslationEntity
    {
        var clonedList = inputList.Select(Mapper.Map<T,T>).ToList();
        var tasks = (from clone in clonedList
            let key = clone.TranslationKey
            where key != null
            select TranslateAsync(key).ContinueWith(t=> setter(clone,t.Result))).ToArray();
        Task.WaitAll(tasks);
        return clonedList;
    }
}

Example call:

var items = itemsList.Localize((i,val) =>i.Description = val);

Again, the setter is just an alternative and implementing that in other ways might be better, but the major point is to avoid reflection whenever possible and if async code is called for multiple items, to await the result of the combined tasks.

Answer 3

There are several points you could address:

1. TranslationKey property

When using something like item.GetType().GetProperty("TranslationKey").GetValue(clone) in a generic method, that means the type is always constained to have a TranslationKey property. I would suggest to use some interface and generic constraint for improving both speed and add a compile-time checks. For example:

public interface ITranslatable
{
     string TranslationKey { get; }
}

usage:

public void Translate<T>(T value) where T : ITranslatable
{
    var translationKey = value.TranslationKey;
    ...
}

This might not be possible to achieve though, especially when using 3rd party types.

2. Retrieving PropertyInfo in each iteration

Your code seems to be always accessing the same property for each element of the collection passed in:

foreach (var item in items)
{
    var clone = Mapper.Map<T, T>();
    ...
        var prop = clone.GetType().GetProperty(propertyName);
    ...
}

You can simply load the property only once for each method call outside of the foreach, this will save some time on the reflection (especially on big collections). If you wanted to optimize for speed further, you could also cache the properties for given type, but I wouldn't recommend it as a starter.

This may not be valid if Mapper.Map<T, T> can actually take and return a type that is more derived than T and hides the propertyName property (e.g. via new keyword).

3. List size

Additional minor point is that you allocate the filteredList without any size hint, yet the size is known, it is the size of inputList. This can save some time on big collections. In addition, the type of inputList parameter can be IReadOnlyCollection, improving the signature to reflect what is being done with it.

LINQ

I would oppose against LINQ usage here because of the prop.SetValue(clone, TranslateAsync(translationKey).Result); side-effect in your foreach loop. You could still return IReadOnlyCollection or IReadOnlyList from the method though, leading to a better flow on usage places.

Final code

public IReadOnlyCollection<T> Localize<T>(IReadOnlyCollection<T> inputList, string propertyName) where T : ITranslatable
{
    var prop = typeof(T).GetProperty(propertyName);

    var filteredList = new List<T>(inputList.Count);

    foreach (var item in inputList)
    {
        var clone = Mapper.Map<T, T>(item);
        // "TranslationKey" is the fixed/same column name for every entity/table.
        var translationKey = clone.TranslationKey;
        if (!string.IsNullOrEmpty(translationKey))
        {
            prop.SetValue(clone, TranslateAsync(translationKey).Result);
        }

        filteredList.Add(clone);
    }
    return filteredList;
}

Answer 4

It seems from your design that TranslationKey can be used to localize exactly one property for each entity, so the following does not make sense:

Localize(itemsList, "Description");
Localize(itemsList, "Title"); // cannot do that, because only description is localizable

If that is the case - you don't need to specify property to localize when calling the method. Instead, use interface like this:

public interface ITranslatable {
    string TranslationKey { get; }
    string TranslatedProperty { get; set; }
}

class MyEntity : ITranslatable {
    public string TranslationKey { get; set; }
    public string Description { get; set; }

    // implicit implementation to avoid clutter
    string ITranslatable.TranslatedProperty
    {
        get { return Description; }
        set { Description = value; }
    }
}

And your Localize method does not need any reflection any more:

public static List<T> Localize<T>(List<T> inputList) where T:ITranslatable
{
    var filteredList = new List<T>();

    foreach (var item in inputList) {
        var clone = Mapper.Map<T, T>(item);
        // "TranslationKey" is the fixed/same column name for every entity/table.                
        if (!string.IsNullOrEmpty(clone.TranslationKey))
            clone.TranslatedProperty = TranslateAsync(clone.TranslationKey).Result;

        filteredList.Add(clone);
    }
    return filteredList;
}

Answer 5

Firstly, using reflection to reset some property is far from being optimized. You probably need to refactor to some kind of Visitor so eventually your code will look like:

new EnglishVisitor.Visit(myObject);

As I see, your classes are responsible for providing some sort of TrnslationKey, right? So next step is to extract appropriate interface: publuc interface ITranslationKeyProvider { string GetTranslationKey(); } ...and this gives you an opportunity to make it more safe with helps of generics: public T Visit<T>(T object) where T : ITranslationKeyProvider so the compiler won't let you to pass something completely different.

It will be much much much more better if you use dynamic keyword to defer type-check up to runtime moment.

Regarding your generics. Thet are perfectly fine.

Answer 6

Can I optimize my methods implementation?

Well, that depends on what you mean by "optimize". I think you can convert your foreach loop into a LINQ Select call followed by a Where call to filter out the nulls.

Is there a way I could not hard code property names when calling the method or is there a better/faster LINQ way or something I am missing?

I suggest using the nameof operator when you pass the property name, so instead of this:

var items = Localize(itemsList, "Description");

You can do this:

var items = Localize(itemsList, nameof(Description));

This way, you can avoid hard coding the strings!