Return to Answer

Using ToString() to compare expressions for equality might work in simple cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.

Using ToString() to compare expressions for equality might work in simple cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.

Using ToString() to compare expressions for equality might works in simple cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.

Using ToString() to compare expressions for equality might work in simple cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.

Using ToString() to compare expressions for equality might works in simples cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.

Using ToString() to compare expressions for equality might works in simple cases, but:

• It requires you to always use the same parameter name, for example, it would consider x => x.Id and product => product.Id to be different expressions.
• Expressions with different meaning can produce the same string, for example (int i) => (float)i and (int i) => (double)i both produce i => Convert(i). Because of this, it might make sense to ensure that the used expressions contain only property accesses and nothing else.

Instead you should compare Expressions properly.

It seems wasteful to me to rebuild all indexes after each change. If you change the collection often, consider changing only the relevant part of each index.

Fields that are set in the constructor and then never modified should be readonly.

IList<T> data

If you're on .Net 4.5, you could use IReadOnlyList<T> here.

if (_lookups.Count > 0)

This check is pretty much useless. It saves you from unnecessarily creating an empty dictionary, but doing that is very cheap, so I think shorter code should take the priority here.

You could replace the whole RebuildIndexes() method with a single ToDictionary():

_indexes = _lookups.ToDictionary(
 lookup => lookup.ToString(), lookup => _items.ToLookup(lookup.Compile()));

c(x).Equals(value)

This won't work correctly when c(x) returns null. You should probably use object.Equals(c(x), value) instead.