28

I have a simple implementation of Fibonacci sequence using BigInteger:

internal class FibonacciEnumerator : IEnumerator<BigInteger>
    {
        private BigInteger _previous = 1;
        private BigInteger _current = 0;

        public void Dispose(){}

        public bool MoveNext() {return true;}

        public void Reset()
        {
            _previous = 1;
            _current = 0;
        }

        public BigInteger Current
        {
            get
            {
                var temp = _current;
                _current += _previous;
                _previous = temp;
                return _current;
            }
        }

        object IEnumerator.Current { get { return Current; }
        }
    }

    internal class FibonacciSequence : IEnumerable<BigInteger>
    {
        private readonly FibonacciEnumerator _f = new FibonacciEnumerator();

        public IEnumerator<BigInteger> GetEnumerator(){return _f;}

        IEnumerator IEnumerable.GetEnumerator(){return GetEnumerator();}
    }

It is an unlimited sequence as the MoveNext() always returns true.

When called using

var fs = new FibonacciSequence();
fs.Take(10).ToList().ForEach(_ => Console.WriteLine(_));

the output is as expected (1,1,2,3,5,8,...)

I want to select 10 items but starting at 100th position. I tried calling it via

fs.Skip(100).Take(10).ToList().ForEach(_ => Console.WriteLine(_));

but this does not work, as it outputs ten elements from the beginning (i.e. the output is again 1,1,2,3,5,8,...).

I can skip it by calling SkipWhile

fs.SkipWhile((b,index) => index < 100).Take(10).ToList().ForEach(_ => Console.WriteLine(_));

which correctly outputs 10 elements starting from the 100th element.

Is there something else that needs/can be implemented in the enumerator to make the Skip(...) work?

rbm
  • 3,243
  • 2
  • 17
  • 28
  • 3
    If you're in the right version of c# (not sure which atm) you can use `yield` and `yield return` which have saved me a lot of effort, and eliminates defining a new enumerator class bc its done for you. – D. Ben Knoble Sep 04 '15 at 19:25
  • 3
    in your case, accessing `Current` several times yields different result. It shouldn't. – njzk2 Sep 04 '15 at 19:33
  • 2
    Uhhh, a getter with side-effects! Nasty. – usr Sep 05 '15 at 15:19

3 Answers3

53

Skip(n) doesn't access Current, it just calls MoveNext() n times.

So you need to perform the increment in MoveNext(), which is the logical place for that operation anyway:

Current does not move the position of the enumerator, and consecutive calls to Current return the same object until either MoveNext or Reset is called.

CodeCaster
  • 147,647
  • 23
  • 218
  • 272
35

CodeCaster's answer is spot on - I'd just like to point out that you don't really need to implement your own enumerable for something like this:

public IEnumerable<BigInteger> FibonacciSequence()
{
  var previous = BigInteger.One;
  var current = BigInteger.Zero;

  while (true)
  {
    yield return current;

    var temp = current;
    current += previous;
    previous = temp;
  }
}

The compiler will create both the enumerator and the enumerable for you. For a simple enumerable like this the difference isn't really all that big (you just avoid tons of boilerplate), but if you actually need something more complicated than a simple recursive function, it makes a huge difference.

Luaan
  • 62,244
  • 7
  • 97
  • 116
6

Move your logic into MoveNext:

public bool MoveNext() 
{
    var temp = _current;
     _current += _previous;
     _previous = temp;
    return true;
}

public void Reset()
{
    _previous = 1;
    _current = 0;
}

public BigInteger Current
{
    get
    {
        return _current;
    }
}

Skip(10) is simply calling MoveNext 10 times, and then Current. It also makes more logical sense to have the operation done in MoveNext, rather than current.

Rob
  • 26,989
  • 16
  • 82
  • 98