Skip to main content
typos
Source Link
svick
  • 24.3k
  • 4
  • 52
  • 89
T ToEntity();

To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity.

protected readonly T EntityType;

EntityType is a bad name for this field, because it does not contain a type, it contains the entity. Because of that, something like Entity might be better.

Also, you might want to consider making this into a property. The reasons for not using public fields also apply to protected fields (though not as strongly).

private IDictionary<string, Action> _propertyNotifications;

This seems completely unnecessary. Unless you know that this actually makes measurable improvement in performance (which I seriously doubt), just raise the event.

var lambda = (LambdaExpression)property;
var unaryExpression = body;

ThisThese two lines are unnecessary and I think they also don't improve readability.

memberExpression = (MemberExpression)unaryExpression.Operand;

If you're expecting only some specific UnaryExpressions, then I would check that those are actually what you have. For example, I think your code would work with () => !BoolProperty, which I think it shouldn't.

T ToEntity();

To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity.

protected readonly T EntityType;

EntityType is a bad name for this field, because it contain type, it contains the entity. Because of that, something like Entity might be better.

Also, you might want to consider making this into a property. The reasons for not using public fields also apply to protected fields (though not as strongly).

private IDictionary<string, Action> _propertyNotifications;

This seems completely unnecessary. Unless you know that this actually makes measurable improvement in performance (which I seriously doubt), just raise the event.

var lambda = (LambdaExpression)property;
var unaryExpression = body;

This two lines are unnecessary and I think they also don't improve readability.

memberExpression = (MemberExpression)unaryExpression.Operand;

If you're expecting only some specific UnaryExpressions, then I would check that those are actually what you have. For example, I think your code would work with () => !BoolProperty, which I think it shouldn't.

T ToEntity();

To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity.

protected readonly T EntityType;

EntityType is a bad name for this field, because it does not contain a type, it contains the entity. Because of that, something like Entity might be better.

Also, you might want to consider making this into a property. The reasons for not using public fields also apply to protected fields (though not as strongly).

private IDictionary<string, Action> _propertyNotifications;

This seems completely unnecessary. Unless you know that this actually makes measurable improvement in performance (which I seriously doubt), just raise the event.

var lambda = (LambdaExpression)property;
var unaryExpression = body;

These two lines are unnecessary and I think they also don't improve readability.

memberExpression = (MemberExpression)unaryExpression.Operand;

If you're expecting only some specific UnaryExpressions, then I would check that those are actually what you have. For example, I think your code would work with () => !BoolProperty, which I think it shouldn't.

Source Link
svick
  • 24.3k
  • 4
  • 52
  • 89

T ToEntity();

To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity.

protected readonly T EntityType;

EntityType is a bad name for this field, because it contain type, it contains the entity. Because of that, something like Entity might be better.

Also, you might want to consider making this into a property. The reasons for not using public fields also apply to protected fields (though not as strongly).

private IDictionary<string, Action> _propertyNotifications;

This seems completely unnecessary. Unless you know that this actually makes measurable improvement in performance (which I seriously doubt), just raise the event.

var lambda = (LambdaExpression)property;
var unaryExpression = body;

This two lines are unnecessary and I think they also don't improve readability.

memberExpression = (MemberExpression)unaryExpression.Operand;

If you're expecting only some specific UnaryExpressions, then I would check that those are actually what you have. For example, I think your code would work with () => !BoolProperty, which I think it shouldn't.