tags:

views:

918

answers:

4

So I have a PropertyBag class that is intended to implement INotifyPropertyChanged. In order to make this code work as cleanly as possible and to avoid user error, I am using the stack to get the property name. See, if the property name doesn't match the actual property exactly, then you will have a failure and I am trying to protect from that.

So, here is an example usage of the class:

public class MyData : PropertyBag
{
    public MyData()
    {
        Foo = -1;
    }

    public int Foo
    {
        get { return GetProperty<int>(); }
        set { SetProperty(value); }
    }
}

The important code for the base PropertyBag is here:

public abstract class PropertyBag : INotifyPropertyChanged
{
    protected T GetProperty<T>()
    {
        string propertyName = PropertyName((new StackTrace()).GetFrame(1));
        if (propertyName == null)
            throw new ArgumentException("GetProperty must be called from a property");

        return GetValue<T>(propertyName);
    }

    protected void SetProperty<T>(T value)
    {
        string propertyName = PropertyName((new StackTrace()).GetFrame(1));
        if (propertyName == null)
            throw new ArgumentException("SetProperty must be called from a property");

        SetValue(propertyName, value);
    }

    private static string PropertyName(StackFrame frame)
    {
        if (frame == null) return null;
        if (!frame.GetMethod().Name.StartsWith("get_") &&
           !frame.GetMethod().Name.StartsWith("set_"))
            return null;

        return frame.GetMethod().Name.Substring(4);
    }
}

So now that you have seen my code, I can tell you the problem... In some cases under release build, the "Foo" setter in the "MyData" constructor appears to be getting optimized to inline as SetProperty(-1). Unfortunately, this inline optimization fails out my SetProperty method because I am no longer calling it from a property! FAIL. It appears that I cannot rely on the StackTrace in this way.

Can anyone A: Figure out a better way to do this but still avoid passing in "Foo" to GetProperty and SetProperty?
B: Figure out a way to tell the compiler to not optimize in this case?

+10  A: 
Marc Gravell
Thanks for the response. That NoInlining attribute is what I was wondering about... but I agree... I won't use it. It requires the users to know to do that, which is worse than using "Foo". Awesome response for many reasons. Thanks.
Brian Genisio
Just to be clear, I was using this mechanism to avoid refactoring problems in the future. If you use a refactoring tool to change the name of the property, then the code will all of a sudden fail, since the name needs to be the same as the property.
Brian Genisio
+2  A: 

If you wish to avoid hard coded strings you can use:

protected T GetProperty<T>(MethodBase getMethod)
{
    if (!getMethod.Name.StartsWith("get_")
    {
        throw new ArgumentException(
            "GetProperty must be called from a property");
    }
    return GetValue<T>(getMethod.Name.Substring(4));
}

Add more sanity checking as you see fit

then the property gets become

public int Foo
{
    get { return GetProperty<int>(MethodInfo.GetCurrentMethod()); }  
}

sets change in the same way.

The GetCurrentMethod() also traverses the stack but does so via an (internal) unmanaged call relying on stack markers so will work in release mode as well.

Alternatively for a quick fix [MethodImpl] with MethodImplAttributes.NoOptimization) or MethodImplAttributes.NoInlining will also work albeit with a performance hit (though given that you are traversing the stack frame each time this hit is negligible).

A further technique, getting some level of compile time checking is:

public class PropertyHelper<T>
{
    public PropertyInfo GetPropertyValue<TValue>(
        Expression<Func<T, TValue>> selector)
    {
        Expression body = selector;
        if (body is LambdaExpression)
        {
            body = ((LambdaExpression)body).Body;
        }
        switch (body.NodeType)
        {
            case ExpressionType.MemberAccess:
                return GetValue<TValue>(
                    ((PropertyInfo)((MemberExpression)body).Member).Name);
            default:
                throw new InvalidOperationException();
        }
    }
}

private static readonly PropertyHelper<Xxxx> propertyHelper 
    = new PropertyHelper<Xxxx>();

public int Foo
{
    get { return propertyHelper.GetPropertyValue(x => x.Foo); }  
}

where Xxxxx is the class in which the property is defined. If the static nature causes issues (threading or otherwise making it an instance value is also possible).

I should point out that these techniques are really for interests sake, I am not suggesting they are good general techniques to use.

ShuggyCoUk
I like this. It makes it so that refactoring the code doesn't cause failures.
Brian Genisio
In reality, though, I think I am going to stick with strings... using MethodInfo is a bit too confusing to the users of the class. As long as there are tests in place, refactoring failures should be handled.
Brian Genisio
see coming edit...
ShuggyCoUk
+1  A: 

Using the stack is not a good idea. You are relying on internal implementation of the compiler to artificially tie in your property bag to the language properties.

  1. having a requirement to add the MethodImpl attribute makes the use of your property bag non-transparent for other developers.
  2. even if the property bag has the MethodImpl attribute, nothing guarantees you it will be the first frame on the call stack. It is possible that the assembly was instrumented or modified to inject calls between the actual property and the call to your property bag. (Think aspect programming)
  3. New languages or even a future version of the C# compiler might decorate the property accessors in a different way then '_get' and '_set'
  4. Constructing the call stack is relatively slow operation, as it requires the internal compressed stack to be decompressed and the name of each type and method to be obtained using reflection.

You should really just implement your property bag accessors to take a parameter to identify the property - either a string name (like Hastable) or an object (like the WPF dependency property bag)

Franci Penov
A: 

You can try to create a T4 template file so your properties could be automatically generated with correct property names for GetProperty() and SetProperty() methods.

T4: Text Template Transformation Toolkit T4 (Text Template Transformation Toolkit) Code Generation - Best Kept Visual Studio Secret

Jozef Izso