Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

This is a fairly trivial example of a question I come up to often. In the example below I tend to think that allowing duplication sometimes results in clearer code.

Does anyone agree/disagree on this point?

First way: an additional local variable is used so method.Invoke is called just once on the last line. I think this has slightly more logic to decode than the second way.

private static void Invoke<T>(string[] args, MethodInfo method) where T : new()
{
    T target = new T();
    target.InvokeMethod("Initialize", errorIfMethodDoesNotExist: false, accessNonPublic: true);
    string[] methodArgs = null;
    if (args.Length > 1)
    {
        methodArgs = args.Skip(1).ToArray();
    }
    method.Invoke(target, methodArgs);
}

Second way: No additional local variable but the call to method.Invoke is duplcated in both branches of the if statement. I think this is clearer even though logic was duplicated.

private static void Invoke<T>(string[] args, MethodInfo method) where T : new()
    {
        T target = new T();
        target.InvokeMethod("Initialize", errorIfMethodDoesNotExist: false, accessNonPublic: true);
        if (args.Length > 1)
        {
            method.Invoke(target, args.Skip(1).ToArray());
        }
        else
        {
            method.Invoke(target, null);
        }
    }
share|improve this question
I personally like your first option in this case as for me it reads easier and is actually less logic in my view to decode as such – dreza May 14 '12 at 23:04

2 Answers

up vote 8 down vote accepted

I think this would be even better, in this case.

private static void Invoke<T>(string[] args, MethodInfo method) where T : new()
{
    T target = new T();
    target.InvokeMethod(
        "Initialize", errorIfMethodDoesNotExist: false, accessNonPublic: true);
    string[] methodArgs = args.Skip(1).ToArray();
    method.Invoke(target, methodArgs);
}

Skip() on an empty collection returns an empty collection.

share|improve this answer
thanks, I did not know that – Aaron Anodide May 14 '12 at 19:13
up vote 0 down vote

or if you want to keep the null value, you can inline the things in the method call with a ternary if like this :

method.Invoke(target, args.Length > 1 ? args.Skip(1).ToArray() : null);

Not super effective here considering svick answer, but in a more general case I think it is preferable to your two options (as long as it doesn't makes you write a 200 characters line).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.