SOLID – A small refactory sequence on ConfArt

I am working on a small library to help working with some dynamic stuff, but the target is mainly to use dynamics to simplify configuration and exchange data when types are not needed. This started as a demo project, but as I used some differents classes and techniques in different projects I tought it should be interesting to share all that in a full functional library and, at least it should be interesting help to learn a few things.

By the way, this is not the target of this first post about it, it’s only about refactoring. As this starts in a demo mode, most of the code was not really well constructed, even if it is almost well tested and most parts were done in TDD mode.

Let’s analyse a method I have:

private  void InitializeSetter()
{
    var expBoxy = _setterExpression.Body;

    if (expBoxy.NodeType == ExpressionType.Convert)
    {
        var convertExp = (UnaryExpression)expBoxy;
        var operand = (MemberExpression)convertExp.Operand;

        if (operand.Member is PropertyInfo)
        {
            PropertyInfo pi = typeof(T).GetProperty(operand.Member.Name);
            _setter = pi.GetValueSetter<T>();
        }

    }
    else if (expBoxy.NodeType == ExpressionType.MemberAccess)
    {
        var operand = (MemberExpression)expBoxy;

        if (operand.Member is PropertyInfo)
        {
            PropertyInfo pi = typeof(T).GetProperty(operand.Member.Name);
            _setter = pi.GetValueSetter<T>();
        }

    }
    else
    {
        throw new InvalidOperationException("Expression is not valid to set a property, should be something like x=>x.Name");
    }
}

This is not too long, but when I get back reviewing the code, it is almost complicated. And why it is complicated because this ends up with too much responsabilities and this does not use the full power of open close principles…

To complete this method, this is also the properties of this class and the constructor:

private Action<T, object> _setter;
private readonly Expression<Func<T, object>> _setterExpression;
private readonly ConfigurationMappingsBase<T> _source;

public Action<string, Action<T, object>> UpdateSource { private get; set; }

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    InitializeSetter();
}

Ok, Let’s analyse the code now.

I get all my properties from the constructor, that’s the good design point, but just in the constructor It begins to fail. It relays too much on ‘Void’ methods to set magicaly stuff and that matters!

As, the goal of InitializeSetter is to initialize the member _setter with some content of the member _setterExpression Let’s do it in another way:

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    _setter = InitializeSetter(_setterExpression);
}

private Action<T, object> InitializeSetter(Expression<Func<T, object>> expression)
{
    var expBoxy = _setterExpression.Body;
    return null; //temp hack
}

Ok, it compiles again and I have no more magical void method stuff, and I should test now this method (But I don’t want to, as I consider it as an internal mechanism and I made it private, but that’s another debate ;-)

But, what do I in the method? I get the Body – which is of type Expression and I work with it. Shouldn’t it be better to pass it directly and avoid a new var?

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    _setter = InitializeSetter(_setterExpression.Body);
}

private Action<T, object> InitializeSetter(Expression expression)
{
    if (expression.NodeType == ExpressionType.Convert)
    {
        var convertExp = (UnaryExpression)expression;
    }
    else if (expression.NodeType == ExpressionType.MemberAccess)
    {
        var operand = (MemberExpression)expression;
    }
    else
    {
        throw new InvalidOperationException("Expression");
    }
}

Now, What did I do with this expression? I rely on my procedural methods deeply printed in my reptilian cortex to make an if-then-else construct to do some work based on the type information of the expression and then cast it to the right type…That’s not too much object oriented no? And more, it violates the OCP…

Let’s rewrite it in an object way:

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    _setter = InitializeSetter(_setterExpression.Body);
}

private Action<T, object> InitializeSetter(UnaryExpression expression)
{
    var operand = (MemberExpression)expression.Operand;

    if (operand.Member is PropertyInfo)
    {
        var pi = typeof(T).GetProperty(operand.Member.Name);
        return pi.GetValueSetter<T>();
    }
    return null;
}

private Action<T, object> InitializeSetter(MemberExpression expression)
{
    if (expression.Member is PropertyInfo)
    {
        PropertyInfo pi = typeof(T).GetProperty(expression.Member.Name);
        _setter = pi.GetValueSetter<T>();
    }
    return null;
}

private Action<T, object> InitializeSetter(Expression expression)
{
    throw new InvalidOperationException("Expression");
}

Well, much better. But, I focuses me much more on what the code do right now, this can be simplified again:

The code of the unary expression, gets the member and do the same code that the member expression one, and as I had to return a value, I had to set to null, this means that my original code should failed If I don’t fall in the right path (or at least my _setter value should be null). Let’s do a last refactor:

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    _setter = InitializeSetter(_setterExpression.Body);
}

private Action<T, object> InitializeSetter(UnaryExpression expression)
{
    return InitializeSetter(expression.Operand);
}

private Action<T, object> InitializeSetter(MemberExpression expression)
{
    if (expression.Member is PropertyInfo)
    {
        var pi = typeof(T).GetProperty(expression.Member.Name);
        return pi.GetValueSetter<T>();
    }
    return InitializeSetter(expression);
}

private Action<T, object> InitializeSetter(Expression expression)
{
    throw new InvalidOperationException("Expression");
}

I am now quite happy with that, and that’s all for the theorical part ;-).

But wait…My tests fail now!

Why do they fail? Because of two things:

  • Expressions don’t rely on an interface but on public abstract class Expression : this means that our _setterExpression.Body will always fall on the last generic method.
  • and because the type PropertyExpression which I need implicitely here is defined as internal class PropertyExpression : MemberExpression

I don’t want to criticize the design of the Linq Expressions namespace, because I can imagine this design has it’s own very good reasons to be like that. But in my simple condition of developper and user of their framework I don’t agree with the way I have to work with it.

That’s why in this real life scenario, I finally end up with this code in order to make tests pass:

public PropertySetterExpression(ConfigurationMappingsBase<T> source, Expression<Func<T, object>> setterExpression)
{
    _source = source;
    _setterExpression = setterExpression;
    _setter = InitializeSetter(_setterExpression.Body);
}

private Action<T, object> InitializeSetter(UnaryExpression expression)
{
    return InitializeSetter(expression.Operand as MemberExpression);
}

private Action<T, object> InitializeSetter(MemberExpression expression)
{
    if (expression.Member is PropertyInfo)
    {
        var pi = typeof(T).GetProperty(expression.Member.Name);
        return pi.GetValueSetter<T>();
    }
    throw new InvalidOperationException("Expression");
}

private Action<T, object> InitializeSetter(Expression expression)
{
    if (expression.NodeType == ExpressionType.Convert)
        return InitializeSetter((UnaryExpression) expression);
    if (expression.NodeType == ExpressionType.MemberAccess)
        return InitializeSetter((MemberExpression) expression);
    throw new InvalidOperationException("Expression");
}

Still better than my very first iteration but not as good as I want.

Any thoughts?

Posted in Articles Tagged with: , , ,

Leave a Reply

Your email address will not be published. Required fields are marked *

*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>