views:

113

answers:

3

I'm writing a abstract file parser (C#) which is extended by two concrete parsers. Both need to perform several checks. Currently there is a validate method in the abstract parser, which uses reflection to call all methods with a name starting with 'test'. That way adding checks is as easy as adding a method with a name that starts with 'test'.

Now recently I've had some comments about the use of reflection and it being better to use dynamic dispatching. My question to you is, why not use reflection and how would you implement this? Also how should I use dynamic dispatch to solve this problem?

    public bool Validate()
    {
        bool combinedResult = true;
        Type t = this.GetType();
        MethodInfo[] mInfos = t.GetMethods();

        foreach (MethodInfo m in mInfos)
        {
            if (m.Name.StartsWith("Check") && m.IsPublic)
            {
                combinedResult &= (bool)m.Invoke(this, null);
            }
        }
        return combinedResult;
    }
+3  A: 

You should use regular OOP and not reflection for this. Have you abstract class expose an abstract method like Validate. Each parser will have to implement it. In the Validate, each parser will call the revelent Check methods to do the work.

Fabian Vilers
Definitely best to stay with standard OOP practices. Reflection is quite useful, but not as efficient as standard function calls.
Zoidberg
Also, when it comes to abstract syntax trees (if your using them) check out the visitor Pattern.
Zoidberg
+2  A: 

There's nothing wrong with code that works... until someone comes to maintain it. In particular, this kind of convention would need to be documented quite carefully as it's not immediately obvious how your class will do what it does.

(As an aside, however, using Reflection for this will be rather slow.)

The most obvious way might be to have an abstract base method bool Validate() that is implemented by the subclasses. The subclasses then have e.g.

public override bool Validate()
{
    return TestFirst() && 
        TestSecond() &&  
        TestThird();
 }

While this looks bulky it's immediately obvious what's going on. It also makes unit testing of Validate() a breeze.

It might also be possible to have the Testxxx() methods register themselves with the superclass in the constructor so they are called automatically - but that's more work and probably less maintainable.

If you really want to do this with reflection, consider marking the Testxxx() methods with attributes and reflecting on those instead. Then your code remains readable.

Jeremy McGee
A: 

As far as I understand, dynamic dispatch is for the cases when you need to determine the method to call based on the type of its arguments. In your case, you call methods with no arguments, so I am not sure what dynamic dispatch has to do with this.

I like your approach for quick-and-dirty stuff. For production-quality code, I see the following potential problems with your approach:

  • you don't check for argument types and return types, so if you or someone adds a method string CheckWithWrongReturnType then your code breaks.
  • every time you call this function it calls GetMethods() and goes through the list. This is probably inefficient. Might be better to cache this list in an array.

To avoid reflection, I would create a delegate and make each class return a list of delegates.

delegate bool Validator();

bool F1() { return true;  }
bool F2() { return false; }

List<Validator> validators = new List<Validator>(F1, F2);

// then in the main class you can do this:

foreach(Validator v in validators)
{
   combinedResult &= v();
}
yu_sha