what is wrong in this code trying to print Fibonacci series in reverse order

Question

tried to take terms in array and printed that array but expected output is different from my output not able to find the logic behind it.

code:-

#include <iostream>
using namespace std;

int main() {
  int num;
  int t1=0;
  int t2=1;
  int next;
  cin>>num;
  int arr[num];

  for (int i = num-1; i >= 0; i--)
  {
    if (i == num-t1-1)
    {
      arr[i] = t1;
      continue;
    }
    if (i == num-t2-1)
    {
      arr[i] = t2;
      continue;
    }
    next = t1+t2;
    arr[i] = next;
    t1 = t2;
    t2 = next;
  }
  
  for(int j=0;j<num;j++)
    {
      cout<<arr[j]<<" ";
    }
    
  return 0;
}

what is wrong in this code? output if user enter 20 is

20
2584 1597 987 610 377 233 144 89 55 34 21 8 13 8 5 3 2 1 1 0

one additional 8 is coming in between 21 and 13.

But expected output should be

20
4181 2584 1597 987 610 377 233 144 89 55 34 21 13 8 5 3 2 1 1 0

C and C++ are two *very* different languages. One difference is that [C++ doesn't have variable-length arrays](https://stackoverflow.com/questions/1887097/why-arent-variable-length-arrays-part-of-the-c-standard). Use `std::vector` instead. — Some programmer dude, Feb 07 '23 at 11:49
As for your problem, generate numbers in the normal order, then *print* in reverse order. Usually much easier. — Some programmer dude, Feb 07 '23 at 11:50

Vlad from Moscow · Accepted Answer · 2023-02-08T09:41:09.070

Your code is a good example of how code should not be written.:)

It is very difficult to understand your code. It's a rule that badly readable code is bound to contain bugs.

For example there are used three variables t1, t2 and next to assign values to elements of the array

arr[i] = t1;
//...
arr[i] = t2;
//...
arr[i] = next;

and it is not clear for which value of the variable i there will be used one of the assignments.

For starters variable length arrays like this

int arr[num];

are not a standard C++ feature. Instead you should use standard container std::vector.

That is if you write a C++ program you should use features provided by the C++ language.

Your code is invalid. For example when i is equal to 11 then t1 is equal tp 8. and this if statement

if (i == num-t1-1)

in this case is equivalent to

if ( 11 == 20-8-1)

or

if ( 11 == 11 )

and the value of t1 is written in the array element arr[11]. instead of writing the value 21.

Here is a demonstration program that shows how your assignment can be performed.

#include <iostream>
#include <functional>
#include <vector>
#include <iterator>

int main()
{
    unsigned int n;

    if (std::cin >> n && n != 0)
    {
        std::vector<unsigned long long> v( n );

        std::pair<unsigned long long, unsigned long long> fibonacci( 0, 1 );

        for (auto first = std::rbegin( v ), last = std::rend( v ); first != last; ++first)
        {
            *first = fibonacci.first;
            fibonacci.first = 
                std::exchange( fibonacci.second, fibonacci.first + fibonacci.second );
        }

        for (const auto &item : v)
        {
            std::cout << item << ' ';
        }
        std::cout << '\n';
    }
}

The program output is the same as expected.

20
4181 2584 1597 987 610 377 233 144 89 55 34 21 13 8 5 3 2 1 1 0

Pay attention that instead of using the signed type int for the variable num you should use an unsigned integer type as for example unsigned int. And to avoid an overflow the variables t1 and t2 should have at least the type unsigned long long int.

In the provided demonstration program this declaration

std::pair<unsigned long long, unsigned long long> fibonacci( 0, 1 );

makes it clear that the program deals with fibonacci numbers.

As a beginner, I am still learning and I value your patience and constructive feedback. thankyou for your time and help. — adarsh tiwari, Feb 07 '23 at 16:29

463035818_is_not_an_ai · Answer 2 · 2023-02-07T12:26:32.907

You want to place t1 and t2 at the last two indices of arr. Those indices are not num-t1-1 and num-t2-1, but num-1 and num-2.

Rather than special casing the loop, it is easier to assign them outside of the loop and start the loop at the 3rd last.

int arr[num]; is not valid C++. Use std::vector for dynamically sized arrays.

Last but not least, the logic in your loop looks too complicated. You only need to use the recurrence relation directly (well, ok reversed) arr[i] = arr[i+1] + arr[i+2], no additional variables are needed.

#include <iostream>
#include <vector>

int main() {
  int num;
  std::cin >> num;

  if (num < 3) {
      std::cout << "num is too small\n";
      return 1;
  }

  std::vector<int> arr(num);

  arr[num-1] = 0;
  arr[num-2] = 1;

  for (int i = num-3; i >= 0; i--) {    
    arr[i] = arr[i+1] + arr[i+2];
  }
  
  for(const auto& e : arr) {
      std::cout << e << " ";
  }       

}

For further reading I refer you to https://en.cppreference.com/w/cpp/container/vector, Why is “using namespace std;” considered bad practice?, Why aren't variable-length arrays part of the C++ standard?,and What is a debugger and how can it help me diagnose problems?.

what is wrong in this code trying to print Fibonacci series in reverse order

But expected output should be

2 Answers2