Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here is a fast recursive Fibonacci-like for loop. How can it be more readable, and is it possible remove TArgs?

public static class Fibonacci
{
    public static BigInteger Calculate(BigInteger number)
    {
        var fibo = AnonRecursiveFiboFunc<BigInteger>(
            func => (step, fibo1, fibo2) => step == number
            ? fibo2
            : func(++step, fibo1 + fibo2, fibo1));
        return fibo(0, 1, 0);
    }

    delegate Func<TArg1, TArg2, TArg3, TArg4>
        Recursive<TArg1, TArg2, TArg3, TArg4>(
        Recursive<TArg1, TArg2, TArg3, TArg4> r);

    private static Func<TArg, TArg, TArg, TArg> 
        AnonRecursiveFiboFunc<TArg>(Func<Func<TArg, TArg, TArg, TArg>,
                                         Func<TArg, TArg, TArg, TArg>> function)
    {
        Recursive<TArg, TArg, TArg, TArg> recursive = 
            rec => (step, fibo1, fibo2) => 
                function(rec(rec))(step, fibo1, fibo2);
        return recursive(recursive);
    }
}
share|improve this question
    
I'm not sure I understand why you are using a delegate instead of using an actual function. It seems to me like this is taking the scenic route instead of the interstate. –  Jeff Vanzella Mar 17 at 19:41
    
i use delegate because i want try it with delegates :) but i dont like multiple TArg's. And i know this is not the best way. I test what can i do with delegates. –  Victor Tomaili Mar 17 at 20:03
1  
I always get confused by this kind of recursion. Is this supposed to be the Y combinator? –  svick Mar 17 at 22:40

3 Answers 3

  1. I like to say T1 T2 instead of TArg1, it just makes it more clear to me what is going on. Your code would look more like this.

    delegate Func<T1, T2, T3, T4> Recursive<T1, T2, T3, T4>( Recursive<T1, T2, T3, T4> r);
    
    private static Func<T, T, T, T> AnonRecursiveFiboFunc<T>(Func<Func<T, T, T, T>, Func<T, T, T, T>> function)
    {
        Recursive<T, T, T, T> recursive = rec => (step, fibo1, fibo2) => function(rec(rec))(step, fibo1, fibo2);
        return recursive(recursive);
    }
    

Other than that... this code seems pretty solid, except for the crazy lack of immediate readability :D and like mentioned in the comments the fact that you are using delegates at all here.

share|improve this answer
1  
Thank you. i will rename TArg's ^^ –  Victor Tomaili Mar 17 at 21:28

The opposite of BenVlodgi's answer: i.e. AnonRecursiveFiboFunc<TArg> has only one template parameter, therefore all the template arguments passed to Recursive<TArg, TArg, TArg, TArg> recursive are the same, therefore there only needs to be one of them.

You can rewrite the code as:

    delegate Func<T, T, T, T> Recursive<T>(Recursive<T> r);

    private static Func<TArg, TArg, TArg, TArg>
        AnonRecursiveFiboFunc<TArg>(Func<Func<TArg, TArg, TArg, TArg>,
                                         Func<TArg, TArg, TArg, TArg>> function)
    {
        Recursive<TArg> recursive =
            rec => (step, fibo1, fibo2) =>
                function(rec(rec))(step, fibo1, fibo2);
        return recursive(recursive);
    }

Given that all the arguments passed to Func are the same, you can reduce it further by defining your own delegate (which I named Function3) as follows:

    delegate T Function3<T>(T arg1, T arg2, T arg3);

    delegate Function3<T> Recursive<T>(Recursive<T> r);

    private static Function3<TArg>
        AnonRecursiveFiboFunc<TArg>(Func<Function3<TArg>,
                                         Function3<TArg>> function)
    {
        Recursive<TArg> recursive =
            rec => (step, fibo1, fibo2) =>
                function(rec(rec))(step, fibo1, fibo2);
        return recursive(recursive);
    }
share|improve this answer
1  
I almost suggested this, but because he was using BigInteger for one type and not for the others, I didn't. Although I guess 3 of them could have indeed been collapsed.... unless I read it wrong o_O –  BenVlodgi Mar 18 at 2:59
    
Ok i see it now :) –  Victor Tomaili Mar 18 at 7:31
    
@BenVlodgi He wasn't using BigInteger for one type and not for the others: BigInteger is the only type because there is only one type, i.e. the type that's passed to the AnonRecursiveFiboFunc<TArg> method, which only has one template parameter. I downvoted your answer because your suggestion made that even less clear, not "more clear" as you said. –  ChrisW Mar 18 at 11:34
    
@ChrisW The only thing I suggested was that he rename his T arguments –  BenVlodgi Mar 18 at 11:43
1  
I already liked @ChrisW's answer. It's good option with yours. –  Victor Tomaili Mar 18 at 12:03

I know you said you liked using delegates, and you wanted to see what you could do with them. But I think its good to recognize when you don't need to make your own.

Here is a simpler refactored version of your code. It does not use your A non recursive function delegate. Also, technically what you were doing before was recursion still.

public static BigInteger Calculate(BigInteger number)
{
    Func<BigInteger, BigInteger, BigInteger, BigInteger> fibo = null;
    fibo = ((step, fibo1, fibo2) => (step == number) ? fibo2 : fibo(++step, fibo1 + fibo2, fibo1));
    return fibo(0, 1, 0);
}
share|improve this answer
    
i use another method because i dont like = null;. –  Victor Tomaili Mar 19 at 9:38
    
@VictorTomaili I didn't like it either... but that is a bit over-kill :D –  BenVlodgi Mar 19 at 11:58

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.