1
\$\begingroup\$

I've just done something that works, but I have the feeling it's not correct. I made a class derived from list. I use a generic type to search the list using one of the properties of the list type as the index. Does this look right to you?

public class IndexableCollection<TCollection, TIndex> : List<TCollection>
{
    public IndexableCollection() : base() {}

    public TCollection this[TIndex index]
    {
        get
        {
            return base.Find(t => t.GetType().GetProperty(index.GetType().Name).GetValue(t).Equals(index));
        }
    }

    public List<TCollection> GetList()
    {
        return this as List<TCollection>;
    }

}

The intention is to have a collection where I can do this:

public class MyClass
{
    public int MyIntProperty { get; set; }
    public string MyStringProperty { get; set; }
    public MyEnumProperty MyEnumProperty { get; set; }
}

public void Main()
{
    var myCollection = new IndexableCollection<MyClass, MyEnumProperty>();

    var element = myCollection[MyEnumProperty.Option1];
}
\$\endgroup\$

closed as off-topic by t3chb0t, Nikita B, Mast, Donald.McLean, pacmaninbw Nov 21 '17 at 14:03

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Please add a more realistic example. \$\endgroup\$ – t3chb0t Nov 15 '17 at 19:57
  • 1
    \$\begingroup\$ As an aside, I would use TElement instead of TCollection, as the generic type refers to the elements of the collection (MyClass), not the collection itself (which is a List). Generic type names, e.g. TFoo, should be read as "type of the Foo". This applies to your TIndex (type of the index), and TElement would be similarly correct. \$\endgroup\$ – Flater Nov 16 '17 at 13:01
  • 2
    \$\begingroup\$ No time for a full review, but it looks like what you're really trying to do can easily be done with Linq: myCollection.FirstOrDefault(item => item.MyEnumProperty == MyEnumProperty.Option1). There's no need for a custom class and reflection (and no mixing up of property and type names). Also, List<T> already uses an indexer, so overloading it with different behavior is confusing. And what if there are multiple matches? With Linq, you could just use Where instead of FirstOrDefault to get all of them. \$\endgroup\$ – Pieter Witvoet Nov 16 '17 at 14:51
4
\$\begingroup\$

The problem

There is a massive hole in your logic here. You are relying on the fact that there exists a property whose type is equal to the name of the property.

public MyEnumProperty MyEnumProperty { get; set; }

Your class would become unusable if I changed this line to:

public MyEnumProperty MyProperty { get; set; }

To explain the issue in detail, the rest of my answer will assume that you're using this second case where the type and name do not match.

The problem is here:

t => t.GetType()
      .GetProperty(index.GetType().Name)  // <== HERE
      .GetValue(t)
      .Equals(index)

GetProperty retrieves a property based on its name (MyProperty). So, what name are you using?

index.GetType().Name

The name of the type (MyEnumProperty). Which is not the same as the MyProperty name that you're looking for.

Let's look at another implementation of your class, where we will use MyIntProperty as the index:

var myCollection = new IndexableCollection<MyClass, int>();

var element = myCollection[5];

Going by the intention of your code, this should be possible. After all, you have made no specific requirements for the given TIndex, which means that your class needs to work for all possible types.

But it doesn't, because when you try to access myCollection[5], you get:

t => t.GetType()
      .GetProperty("Int32")
      .GetValue(t)
      .Equals(index)

There is no property by the name of Int32, so it does not work.


The solution

In essence, you are trying to define the indexed property by its type alone, which is a dangerous thing to do.

We could rewrite the code to search MyClass for the first property of the given type TIndex. However, what will you do if MyClass has multiple properties of that same type?

You'd be stuck guessing at the intended index, and that's not a good solution either.

What we're missing is the definition of the exact property you want to use as an index. You're trying to find it by its type alone, and that is not enough. Instead, you need a way to select the correct property.

My solution: the lambda.

You need a mapping between the element (TElement) and its intended index (TIndex). This is the perfect situation to use a lambda method of type Func<TElement, TIndex>.

I updated your code to a working example with lambdas. I slightly tweaked your code for the sake of example:

public enum MyEnum { One, Two, Three }

public class MyClass
{
    public int MyIntProperty { get; set; }
    public string MyStringProperty { get; set; }
    public MyEnum MyEnumProperty { get; set; }
}

The collection class:

public class MyCollection<TElement, TIndex> : List<TElement>
{
    private Func<TElement, TIndex> _indexMapping { get; set; }

    public MyCollection(Func<TElement, TIndex> indexMapping)
    {
        _indexMapping = indexMapping;
    }

    public TElement this[TIndex index]
    {
        get
        {
            return base.Find(t => _indexMapping.Invoke(t).Equals(index));
        }
    }
}

The only real difference is that we now store a Func<TElement, TIndex> _indexMapping. This gives us the mapping between the element and its index.
This has the interesting side effect that you can't use a TIndex that doesn't exist as a property in TElement (unless you supply a constant value unrelated to the element, but then you're being silly).

base.Find(t => _indexMapping.Invoke(t).Equals(index));

When iterating over the list, we simply invoke the mapping (on the element t), which returns the value of its intended index. All we have to do then is check whether the value matches the index that was currently given as a parameter, which is easily done by calling .Equals().

Example usages:

//Enum as index

var myCollection = new MyCollection<MyClass, MyEnum>(o => o.MyEnumProperty);
var element = myCollection[MyEnum.One];

//Int as index

var myCollection = new MyCollection<MyClass, int>(o => o.MyIntProperty);
var element = myCollection[12345];

//String as index

var myCollection = new MyCollection<MyClass, String>(o => o.MyStringProperty);
var element = myCollection["Hello"];

Tangential comments

  • If your index is a custom class, make sure to implement the correct equality comparison! The default behavior (checking if it's the same object in memory) may or may not be what you want.
  • There's still an issue when your collection contains multiple items with the same index. You'll always receive the first matching item, the others will essentially be invisible (until you remove the first item). Again, depending on your requirements, that might be intentional behavior, or it might not be.
    • If you want to ensure that you only have unique items in the list, and no duplicate indexes exist, you will need additional logic to validate new entries to the list.
  • As an aside, I would use TElement instead of TCollection, as the generic type refers to the elements of the collection (MyClass), not the collection itself (which is a List). Generic type names, e.g. TFoo, should be read as "type of the Foo". This applies to your TIndex (type of the index), and TElement would be similarly correct.
\$\endgroup\$
  • \$\begingroup\$ I cannot thank you enough for having taken the time to review my question and provided such a detailed answer. I knew something wasn't right with what I'd wrote, but I don't have the expertise to have come up with a solution like the one you propose. I'm busy now implementing this solution. Thanks again. \$\endgroup\$ – Carlos Gamez Nov 17 '17 at 13:12
  • \$\begingroup\$ @CodeRecipes: No worries, happy to help :) \$\endgroup\$ – Flater Nov 17 '17 at 13:40
1
\$\begingroup\$

You are probably better off by using @Flater's answer or @PieterWitvoet comment. Querying with LINQ is easy and makes the intention apparent, while your approach is very uncommon.

If you still want to stick to your solution using reflection, there is an improvement you can make. In ...

public TCollection this[TIndex index]
{
    get
    {
        return base.Find(t => t.GetType().GetProperty(index.GetType().Name)
                              .GetValue(t).Equals(index));
    }
}

... the extraction of the property info can be made in advance, since it will always be the same.

Note that t.GetType().GetProperty(index.GetType().Name) in your implementation will be called in a loop inside Find.

public class IndexableCollection<TElement, TIndex> : List<TElement>
{
    private PropertyInfo _property;

    public IndexableCollection()
    {
        _property = typeof(TElement).GetProperty(typeof(TIndex).Name);
    }

    public TCollection this[TIndex index]
    {
        get
        {
            return Find(t => _property.GetValue(t).Equals(index));
        }
    }

    ...
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time. As you've mentioned, I think @Flater solution is pretty good. I'm going to try to use that one. \$\endgroup\$ – Carlos Gamez Nov 17 '17 at 13:14

Not the answer you're looking for? Browse other questions tagged or ask your own question.