3
\$\begingroup\$

I have a Vehicle object which has a few properties such as Make, Model, Price, etc.

I also have a VehicleCollection object which derives from List<Vehicle which has a few custom methods, one of which is the following...

class VehicleCollection : List<Vehicle>
{
    public VehicleCollection GetVehicles(string _searchTerm = "")
    {
        // return the entire collection if no search term is provided
        if (_searchTerm.Length == 0)
            return this;

        var matchingVehicles = new VehicleCollection();
        foreach(var vehicle in this) // loop through all vehicles
        {
            PropertyInfo[] propInfos = vehicle.GetType().GetProperties(); // all properties of the current vehicle
            for (int i = 0; i < propInfos.Length; i++)
            {
                var propVal = propInfos[i].GetValue(vehicle); // the value of the current property of the current vehicle

                // check aganst value of a strings or the values within a string array
                // ignores case
                if (propVal is string propertyValueStr && propertyValueStr.IndexOf(_searchTerm, StringComparison.CurrentCultureIgnoreCase) >= 0
                    || propVal is string[] propValueArr && propValueArr.Any(x => x.IndexOf(_searchTerm, StringComparison.CurrentCultureIgnoreCase) >= 0))
                {
                    matchingVehicles.Add(vehicle);
                }
            }
        }

        return matchingVehicles;
    }
}

This returns a collection of vehicles, of which each vehicle must have a property whose value contains the supplied _searchTerm parameter.

Is this the best way to do this? Can this be achieved using LINQ at all? Can this be done without reflection?

Edit: At the request of a comment, here is the Vehicle Class, with a few unnecessary methods omitted.

public class Vehicle
{
    public string ID { get; private set; }
    public string Make { get; private set; }
    public string Model { get; private set; }
    internal decimal Value { get; private set; }
    public string Price { get { return string.Format("{0:C}", Value); } }
    public VehicleColourEnum Colour { get; private set; }
    internal StatusEnum Status { get; private set; }
    public string State
    {
        get 
        {
            switch (Status)
            {
                case StatusEnum.ComingSoon:
                    return "Coming Soon";
                case StatusEnum.HasDeposit:
                    return "Has Deposit";
                case StatusEnum.InStock:
                    return "In Stock";
                case StatusEnum.Sold:
                    return "Sold";
                case StatusEnum.Unknown:
                default:
                    return "Unknown";
            }
        }
    }

    public Vehicle(string _make, string _model, decimal _value, VehicleColourEnum _colour = VehicleColourEnum.None, StatusEnum _status = StatusEnum.Unknown)
    {
        Make = _make;
        Model = _model;
        Value = _value;
        Colour = _colour;
        Status = _status;
        ID = Guid.NewGuid().ToString("N");
    }
}
\$\endgroup\$
  • \$\begingroup\$ @Heslacher Added the Vehicle class, and this would just be the current instance of the VehicleCollection class, which is a List<Vehicle>. Hopefully that helps and is what you were looking for. \$\endgroup\$ – Danny Goodall Mar 20 '18 at 10:55
4
\$\begingroup\$

I'll ignore your naming conventions/etc as I know that's not what you're asking about.

Since the VehicleCollection object already inherits from IEnumerable through the List<> inheritence, you can query the collection as a list through the 'this' keyword.

That means you could write up your search method like:

public VehicleCollection GetVehicles(string searchTerm = "")
{
    // Null and white-space check for sanity
    if(string.IsNullOrWhiteSpace(searchTerm))
    // We're assuming if the search term is bad, we return everything. 
    // This isn't standard practice - usually we return nothing
        return this; 

    var matches = new VehicleCollection();
    matches.AddRange(
        this.Where(v=>v.ID.Equals(searchTerm) || 
                    v.Make.Equals(searchTerm) || 
                    v.Model.Equals(searchTerm)
    // I personally prefer to be explicit in what I'm selecting, but it's your choice
        ).Select(v=>v)); 

    // inline IF because I'm lazy
    // again, we're returning everything if there are no matches, 
    // which isn't standard practice.
    return matches.Any()? matches: this; 
}

Hope that helps!

\$\endgroup\$
  • \$\begingroup\$ Good answer, I like the point about the standard practice being to return nothing, which makes sense. I chose to use reflection simply because it means that if any more properties are added to Vehicle, I don't have to update this method too. \$\endgroup\$ – Danny Goodall Mar 20 '18 at 13:30
  • \$\begingroup\$ While yes, you're de-coupling the search method from the entity you're searching, the two should be considered tightly coupled anyway. In the context of vehicles outside of the code base, if you are looking for 'Red', you have a concept of what Red means and applies to (vehicle color), rather than looking at all the properties of all the vehicles and hoping that 'Red' will turn up. Your way with reflection may work, though I imagine it has potential to throw up results you don't want while slowing down the overall search process. \$\endgroup\$ – Danielle Summers Mar 20 '18 at 13:39
  • 1
    \$\begingroup\$ Regarding the explicitness of selection: your code is no more explicit than leaving the statement is off. You’re merely adding a tautology. It adds nothing of value, and in fact decreases the signal-to-noise ratio. \$\endgroup\$ – Konrad Rudolph Mar 20 '18 at 15:15
  • \$\begingroup\$ I'm not going to lie, I had to google Tautology, and I'm still not certain I fully grasp the concept, but thanks for the reply. It's something I'll keep in mind \$\endgroup\$ – Danielle Summers Mar 20 '18 at 15:29
4
\$\begingroup\$

If you do find a matching vehicle, you add it. Then you continue checking on the same vehicle instead of breaking out of the inner loop to fetch the next vehicle.

Reflection is sluggish. I would suggest having the Vehicle class determine if its matches a search string. That would mean having a method such as:

public bool IsMatch(string value) ...

Then your looping over all the vehicles becomes much simpler, and faster too.

\$\endgroup\$
  • \$\begingroup\$ Ahh good shout about the breaking the inner loop thing, thanks. \$\endgroup\$ – Danny Goodall Mar 20 '18 at 13:27
3
\$\begingroup\$

While possibly overkill, your use of reflection and request for linq got me thinking about building the expression dynamically.

I believe that the GetVehicles method on the collection could be delegated out, but using a simple extension method that followed your initial attempt at filtering the properties I came up with

public static class ExpressionBuilder {

    public static IEnumerable<T> Match<T>(this IEnumerable<T> collection, string filter = null) where T : class {
        // return the entire collection if no search term is provided
        if (string.IsNullOrWhiteSpace(filter))
            return collection;

        var lambda = ExpressionBuilder.IsMatch<T>(filter);

        var matches = collection.Where(lambda.Compile());

        return matches;
    }

    static Expression<Func<T, bool>> IsMatch<T>(string filter) where T : class {
        var type = typeof(T);
        var properties = type.GetProperties();

        // (T _) => ...
        var param = Expression.Parameter(type, "_");
        //filter
        var filterConstant = Expression.Constant(filter);
        // null
        var nullString = Expression.Constant(null, typeof(string));

        Expression body = null;

        foreach (var propertyInfo in properties) {
            var propertyType = propertyInfo.PropertyType;
            if (propertyType == typeof(string)) {
                // _.Property
                var property = Expression.Property(param, propertyInfo);
                // _.Property != null
                var notNull = Expression.NotEqual(property, nullString);
                // _.Property.IndexOf(filter, StringComparison.CurrentCultureIgnoreCase)
                var method = Expression.Call(
                    property,
                    propertyType.GetMethod("IndexOf", new[] { typeof(string), typeof(StringComparison) }),
                    filterConstant,
                    Expression.Constant(StringComparison.CurrentCultureIgnoreCase)
                );
                // _.Property.IndexOf(filter, StringComparison.CurrentCultureIgnoreCase) >= 0
                var contains = Expression.GreaterThanOrEqual(method, Expression.Constant(0));
                // _.Property != null && _.Property.IndexOf(filter, StringComparison.CurrentCultureIgnoreCase) >= 0
                var condition = Expression.AndAlso(notNull, contains);

                if (body == null) {
                    body = condition;
                } else {
                    body = Expression.Or(body, condition);
                }
            } else if (propertyType.IsValueType) {
                // _.Property
                var property = Expression.Property(param, propertyInfo);
                // _.Property.ToString()
                var method = Expression.Call(
                    property,
                    propertyType.GetMethod("ToString", new Type[0])
                );
                // _.Property.ToString().IndexOf(filter, StringComparison.CurrentCultureIgnoreCase)
                method = Expression.Call(
                    method,
                    method.Method.ReturnType.GetMethod("IndexOf", new[] { typeof(string), typeof(StringComparison) }),
                    filterConstant,
                    Expression.Constant(StringComparison.CurrentCultureIgnoreCase)
                );
                // _.Property.ToString().IndexOf(filter, StringComparison.CurrentCultureIgnoreCase) >= 0
                var contains = Expression.GreaterThanOrEqual(method, Expression.Constant(0));
                var condition = contains;

                if (body == null) {
                    body = condition;
                } else {
                    body = Expression.Or(body, condition);
                }
            }
        }

        var lambda = Expression.Lambda<Func<T, bool>>(body, param);

        return lambda;
    }
}

Which builds an expression tree that predicates on the provided object type to be used with Linq extension.

For example

public class VehicleCollection : List<Vehicle> {

    public VehicleCollection()
        : base() {

    }

    public VehicleCollection(IEnumerable<Vehicle> collection)
        : base(collection) {

    }

    public VehicleCollection GetVehicles(string _searchTerm = "") {
        // return the entire collection if no search term is provided
        if (string.IsNullOrWhiteSpace(_searchTerm))
            return this;

        var matches = this.Match(_searchTerm); //<-- calling extension method

        var matchingVehicles = new VehicleCollection(matches);

        return matchingVehicles;
    }
}

The extension method is an initial attempt and there is room for improvement in terms of what the predicate can handler. This should however be enough to get you started.

\$\endgroup\$
  • \$\begingroup\$ I like your way of documenting expressions... I'll borrow it ;-] \$\endgroup\$ – t3chb0t Mar 20 '18 at 16:40
  • \$\begingroup\$ @t3chb0t it helps keep the picture of what I am creating. It is very easy to get confused. This is the Only way it makes sense to me. \$\endgroup\$ – Nkosi Mar 20 '18 at 16:54

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.