5
\$\begingroup\$

I have a method which takes a certain generic object, which basically orders these objects based on a list of tags. Because I want to use this method for multiple objects, I have created a class which only contains the Id, list of tags and an average rating from each object, it looks like this:

public class RecommenderContentItem
{
    public Guid Id { get; set; }
    public List<Tag> Tags { get; set; } //A tag is an object which only contains an Id and value, not relevant to the question.
    public double AverageRating { get; set; }
}

Now I have tried to create a generic method which takes T and checks which type it is, before converting it. The possible objects it can filter are Restaurant, AlgorithmRestaurant and Dish. All of these classes contain an Id, list of tags and an average. It looks like this:

public static List<Guid> FilterOnContent<T>(List<T> ratedItems,
        List<T> itemsToFilter)
{
    var cnv = new RciConverter();

    var ratedRcis = new List<RecommenderContentItem>();
    var toFilterRcis = new List<RecommenderContentItem>();
    if (typeof(T) == typeof(Restaurant))
    {
        var rated = (IEnumerable<Restaurant>)ratedItems;
        var toFilter = (IEnumerable<Restaurant>)itemsToFilter;

        ratedRcis = cnv.ConvertMany(rated).ToList();
        toFilterRcis = cnv.ConvertMany(toFilter).ToList();
    }
    if (typeof(T) == typeof(AlgorithmRestaurant))
    {
        var rated = (IEnumerable<AlgorithmRestaurant>)ratedItems;
        var toFilter = (IEnumerable<AlgorithmRestaurant>)itemsToFilter;

        ratedRcis = cnv.ConvertMany(rated).ToList();
        toFilterRcis = cnv.ConvertMany(toFilter).ToList();
    }
    if (typeof(T) == typeof(Dish))
    {
        var rated = (IEnumerable<Dish>)ratedItems;
        var toFilter = (IEnumerable<Dish>)itemsToFilter;

        ratedRcis = cnv.ConvertMany(rated).ToList();
        toFilterRcis = cnv.ConvertMany(toFilter).ToList();
    }
    if (!ratedRcis.Any() || !toFilterRcis.Any())
        throw new TypeArgumentException("Invalid type."); //Custom exception written by Jon Skeet.

    return ContentBasedFilter.Filter(ratedRcis, toFilterRcis).Select(rci => rci.Id).ToList();
}

The converter class above has an interface, and converts (or maps) the object to an RecommenderContentItem. It looks like this:

public class RciConverter : IConverter<Restaurant, RecommenderContentItem>,
    IConverter<AlgorithmRestaurant, RecommenderContentItem>,
    IConverter<Dish, RecommenderContentItem>
{
    //For each method, I have to call ToList() on the tag list, since they're ICollections
    public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<AlgorithmRestaurant> sourceObjects)
    {
        return sourceObjects.Select(rest => new RecommenderContentItem
        {
            Id = rest.RestaurantId,
            AverageRating = rest.AverageRating,
            Tags = rest.Tags.ToList()
        });
    }

    public RecommenderContentItem Convert(AlgorithmRestaurant sourceObject)
    {
        return new RecommenderContentItem
        {
            Id = sourceObject.RestaurantId,
            AverageRating = sourceObject.AverageRating,
            Tags = sourceObject.Tags.ToList()
        };
    }

    public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<Dish> sourceObjects)
    {
        return sourceObjects.Select(dish => new RecommenderContentItem
        {
            Id = dish.Id,
            AverageRating = dish.AverageRating,
            Tags = dish.Tags.ToList()
        });
    }

    public RecommenderContentItem Convert(Dish sourceObject)
    {
        return new RecommenderContentItem
        {
            Id = sourceObject.Id,
            AverageRating = sourceObject.AverageRating,
            Tags = sourceObject.Tags.ToList()
        };
    }

    public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<Restaurant> sourceObjects)
    {
        return sourceObjects.Select(rest => new RecommenderContentItem
        {
            Id = rest.Id,
            AverageRating = rest.AverageRating,
            Tags = rest.Tags.ToList()
        });
    }

    public RecommenderContentItem Convert(Restaurant sourceObject)
    {
        return new RecommenderContentItem
        {
            Id = sourceObject.Id,
            AverageRating = sourceObject.AverageRating,
            Tags = sourceObject.Tags.ToList()
        };
    }
}

And the interface:

public interface IConverter<in TSource, out TDestination>
    where TSource : class
    where TDestination : class
{
    IEnumerable<TDestination> ConvertMany(IEnumerable<TSource> sourceObjects);
    TDestination Convert(TSource sourceObject);
}

Now my question is, am I doing this the right way? I haven't worked a lot with generics or C# in general, so I'm not sure if there are better ways to achieve what I'm trying to accomplish. If you need more information about classes or functionality, please let me know so I can add it to my post!

\$\endgroup\$
  • \$\begingroup\$ +1 for the exception but "Jon Skeet"* (without the h) --> stackoverflow.com/users/22656/jon-skeet \$\endgroup\$ – Ivaro18 Jan 20 '17 at 11:17
  • \$\begingroup\$ Can you show the implementation of the 6 mapping methods ? \$\endgroup\$ – Denis Jan 20 '17 at 12:15
  • 1
    \$\begingroup\$ @denis added them to my post! \$\endgroup\$ – RandomStranger Jan 20 '17 at 12:18
2
\$\begingroup\$

Improving List<Guid> FilterOnContent<T>

You should use if/else structure when you need only a single condition to be triggered instead of multiple if's as the compiler will have to go over all of them, but that's not necessary because let's say typeof(T) == typeof(Restaurant) do you want to check typeof(T) == typeof(AlgorithmRestaurant) and the rest of your conditions? There is no point the type argument cant be 2 types at the same time.

var rated = (IEnumerable<AlgorithmRestaurant>)ratedItems;
var toFilter = (IEnumerable<AlgorithmRestaurant>)itemsToFilter;

Those helper variables seem pointless to me they just add 2 extra lines in each if branch.

The .ToList() also adds a few extra characters, while you can just call it once at the return statement.

You currently have only if statements so you cant guarantee for the compiler that there will always be exactly one condition that will be met, which means you need to allocate some extra memory at initialization:

var ratedRcis = new List<RecommenderContentItem>();

But with multiple else if's and a one else for the exception you wont need to do that.

The empty .Any() overload is O(1) operation but with else at the end you wont even need to do that.

With all of that in mind you can make your method look like this:

public static List<Guid> FilterOnContent<T>(List<T> ratedItems, List<T> itemsToFilter)
{
    RciConverter cnv = new RciConverter();

    IEnumerable<RecommenderContentItem> ratedRcis;
    IEnumerable<RecommenderContentItem> toFilterRcis;
    if (typeof(T) == typeof(Restaurant))
    {
        ratedRcis = cnv.ConvertMany((IEnumerable<Restaurant>) ratedItems);
        toFilterRcis = cnv.ConvertMany((IEnumerable<Restaurant>) itemsToFilter);
    }
    else if (typeof(T) == typeof(AlgorithmRestaurant))
    {
        ratedRcis = cnv.ConvertMany((IEnumerable<AlgorithmRestaurant>) ratedItems);
        toFilterRcis = cnv.ConvertMany((IEnumerable<AlgorithmRestaurant>) itemsToFilter);
    }
    else if (typeof(T) == typeof(Dish))
    {
        ratedRcis = cnv.ConvertMany((IEnumerable<Dish>) ratedItems);
        toFilterRcis = cnv.ConvertMany((IEnumerable<Dish>) itemsToFilter);
    }
    else
    {
        throw new TypeArgumentException("Invalid type."); //Custom exception written by Jon Skeet.
    }
    return ContentBasedFilter.Filter(ratedRcis.ToList(), toFilterRcis.ToList()).Select(rci => rci.Id).ToList();
}

In fact there isn't really a pretty way to shorten that, not much conversion is available to generics and since you need to know the type of casting at compile time you cant go fancy-mancy.

Shortening your mapping methods

Why would you write the same code in your ConvertMany when you already have it in the same method for a single instance Convert?

public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<AlgorithmRestaurant> sourceObjects)
{
    return sourceObjects.Select(rest => new RecommenderContentItem
    {
        Id = rest.RestaurantId,
        AverageRating = rest.AverageRating,
        Tags = rest.Tags.ToList()
    });
}

public RecommenderContentItem Convert(AlgorithmRestaurant sourceObject)
{
    return new RecommenderContentItem
    {
        Id = sourceObject.RestaurantId,
        AverageRating = sourceObject.AverageRating,
        Tags = sourceObject.Tags.ToList()
    };
}

It doesn't makes sense you can just do:

public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<AlgorithmRestaurant> sourceObjects)
{
    return sourceObjects.Select(rest => Convert(rest));
}

And even shorter with a method group:

public IEnumerable<RecommenderContentItem> ConvertMany(IEnumerable<AlgorithmRestaurant> sourceObjects)
{
    return sourceObjects.Select(Convert);
}

You can apply the same to all of your other ConvertMany methods.

Lastly I'm not sure why do you need everything to be in a List<T> if you don't have a really good reason to use them, you shouldn't call .ToList() everywhere if you data is huge enumerating it again would surely cause a bottleneck there.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for such an elaborate answer! What exactly do you mean with your last part? Just for the return statement? I'll go through my code, and let you know when any questions arise. \$\endgroup\$ – RandomStranger Jan 20 '17 at 12:46
  • \$\begingroup\$ @RandomStranger Notice the .ToList() call in the return statement ratedRcis.ToList(), toFilterRcis.ToList() we call it here once instead of everywhere in your if statements. \$\endgroup\$ – Denis Jan 20 '17 at 12:49
  • \$\begingroup\$ I get what you're saying, but since it's only possible to reach 1 of the if/else paths, it should only call .ToList() once, right? Does it matter when you call it at the end or not? \$\endgroup\$ – RandomStranger Jan 20 '17 at 12:50
  • \$\begingroup\$ @RandomStranger It's just to shorten the line, the call will always occur once you are correct. \$\endgroup\$ – Denis Jan 20 '17 at 12:51
  • \$\begingroup\$ I see, thank you! I'll also go through my code and remove as many lists as I can, I didn't realise this was such a bottleneck \$\endgroup\$ – RandomStranger Jan 20 '17 at 12:52
2
\$\begingroup\$
public RecommenderContentItem Convert(AlgorithmRestaurant sourceObject)
{
  return new RecommenderContentItem
  {
      Id = sourceObject.RestaurantId,
      AverageRating = sourceObject.AverageRating,
      Tags = sourceObject.Tags.ToList()
  };
}

public RecommenderContentItem Convert(Dish sourceObject)
{
  return new RecommenderContentItem
  {
      Id = sourceObject.Id,
      AverageRating = sourceObject.AverageRating,
      Tags = sourceObject.Tags.ToList()
  };
}

public RecommenderContentItem Convert(Restaurant sourceObject)
{
  return new RecommenderContentItem
  {
      Id = sourceObject.Id,
      AverageRating = sourceObject.AverageRating,
      Tags = sourceObject.Tags.ToList()
  };
}

Look at these three methods. What do you see? They are virtually identical. In fact you don't need them at all because all types that you convert to RecommenderContentItem have the same three properties. All you need is just this interface that all three classes implement. Alternatively you can make the RecommenderContentItem an abstract class and the base class for the other types.

public interface IRecommenderContentItem
{
    Guid Id { get; set; }
    List<Tag> Tags { get; set; }
    double AverageRating { get; set; }
}

Then you can remove the most part of the FilterOnContent method and constrain T to be of that interface type:

public static List<Guid> FilterOnContent<T>(List<T> ratedItems, List<T> itemsToFilter) 
    where T : IRecommenderContentItem
{   
    if (!ratedItems.Any() || !itemsToFilter.Any())  throw new ArgumentException("Invalid type."); 

    return ContentBasedFilter.Filter(ratedRcis, toFilterRcis).Select(rci => rci.Id).ToList();
}

The ContentBasedFilter.Filter will need to be adjusted too but you didn't include it so I cannot say anything about it.

\$\endgroup\$
  • \$\begingroup\$ I like this approach, I have brought this up with the other developers, but I wasn't allowed to include an interface for the models. My opinion doesn't weigh that much as an intern.. Still, this is a good solution! \$\endgroup\$ – RandomStranger Jan 20 '17 at 14:34
  • \$\begingroup\$ @RandomStranger I wasn't allowed to include an interface for the models. really? Is there any particular reason for this? \$\endgroup\$ – t3chb0t Jan 20 '17 at 14:39
  • \$\begingroup\$ I know... It's probably because they consider me 'just the intern'. They consider the algorithm I'm working on a joke, and I'd be surprise if this ends up in the application. I'm done in a week and a little bit, so I'm only worried about getting a good grade for this internship. But all of that aside, your answer is still valid and works well! \$\endgroup\$ – RandomStranger Jan 20 '17 at 14:41

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.