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.
#include <iostream>
#include <vector>

void getUserNum(int &);
int rFib(int n);
void dispSeq(std::vector<int> &vRef);

int main()
{
    int userNum;
    getUserNum(userNum);
    rFib(userNum);  
    return 0;
}

void getUserNum(int &refVal)
{
    std::cout << "Enter a positive integer:\n";
    std::cin >> refVal;
}

int rFib(int n)
{
     static std::vector<int> fib;
     static bool baseReached = false;
     fib.push_back(n);

     if (baseReached)
     {
             dispSeq(fib);
     }

     if (n <= 1)
     {
        baseReached = true;
        return n;
     }
     else
     {
        return rFib(n-2) + rFib(n-1);
     }
 }

void dispSeq(std::vector<int> &v)
{
    for (size_t i = 0; i < v.size(); i++)
    {
        std::cout << v[i] << " ";
    }
    std::cout << std::endl;
 }
share|improve this question
2  
Asking how to do something is best done on SO. Though a good code review here will definitely push you in the correct direction. –  Loki Astari Oct 4 '13 at 16:58
add comment

closed as off-topic by 200_success, Jamal, Yuushi, Vedran Šego, Brian Reichle Oct 5 '13 at 11:41

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After your code is working you can edit this question for reviewing your working code." – 200_success, Jamal, Yuushi, Vedran Šego
If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

Its normal to put parameter names even in the declarations:

void getUserNum(int &);

We use this to help us identify what the function is doing.

In C++ the '*' and the '&' are part of the type information. So it is usual to push these left towards the type rather than the name. Note this is basically the opposite of C conventions. But C++ is much more rigorous on types and safety than C.

void dispSeq(std::vector<int> &vRef);

// Normally in C++ code I would expect this:
void dispSeq(std::vector<int>& vRef);

Be careful with your use of out parameters.

void getUserNum(int &refVal)

It seems more natural to return a value from a function then get sombody inside to set an external value via a reference (not unheard of and it will depend a lot on the context). But in this situation I would return a value. It will then make using the code eaiser.

 int userNum   = getUserNum();

Error Checking (especially input from a human) needs to be more rigorous.

void getUserNum(int &refVal)
{
    std::cout << "Enter a positive integer:\n";
    std::cin >> refVal;
}

If a user enters a non positive value things will go bad. If the user inputs a string the value of refVal I believe is left unaltered (which leaves your code with an uninitialized variable).

int getUserNum()
{
    int result;

    // user input is done by the line.
    // so read a line at a time and validate it.
    std::string  line;
    std::getline(std::cin, line);

    std::stringstream  linestream(line);

    int result;
    if (linestream >> result)
    {
         // The read worked.
         // Lets check the user did not type ofther crap on the line.
         //
         char x;
         if (!(linestream >> x))
         {
               // Reading more data worked. This means the input was good.
               // As the user typed a number followed by some other stuff
               // like  56.4   (the .4 is illegal)
               //       5Plop  (the Plop is illegal)
               //
               // The the above would have successfully read a value into
               // `x` this with the ! it would have failded and this
               // this condition would not have been entered.

               // We only return if:
               //    1. We read an integer. 
               //    2. There is no other data on the line except white space.
               return result;
         }
    }
    // invalid input
    std::cout << "Stupid user input aborting\n";
    exit(1); // Reading failed.
             // Leave as an exercise on how to repeat the question.
             // You can also compress that very verbose if statement into a single
             // line without it looking too bad.
}

// Single line if
if ((linestream >> result) && !(linestream >> x))
{   return result;
}

In Fib() you should probably not use recursion. Prefer a while loop. Loop from 0 upto the point you want to reach. This makes using a vector easy. You can also start from where you left off last time rather than 0.

int fib(int n)
{
     static std::vector<int>   values = {1,1};
     if (n < values.size()-1) }return values[n];

     int start = values.size();
     for(int loop = start; loop < n; ++loop)
     {
          values[loop] = values[loop-2] + values[loop-1];
     }
     return values[n];
}
share|improve this answer
    
Thank you very much! Amazing advice. –  ao2130 Oct 5 '13 at 3:41
add comment

@Loki Astari has covered the important points, so I won't repeat those. I'll just address recommended integer and size types:

  • For getUserNum(), you're specifically asking the user for a positive number. So, instead of reading into an int, use an unsigned int. You should still include validation. This also better reveals your intent to the reader.

    You could also instead have main() get this input. That way, if you don't want to hound the user for proper input, you could just return EXIT_FAILURE if the first attempt fails.

  • size_t is not quite close enough for std::vector (and std::size_t is more C++-like anyway). Although it is an STL container, it's best not to assume that each one is of this type. For all STL containers, prefer size_type as it's more portable:

    standardContainerType::size_type;
    

    For your vector:

    std::vector<int>::size_type;
    

    In fib()'s for-loop, use this type for i:

    // this could go before the loop
    //   if it looks too long to go inside
    std::vector<int>::size_type i;
    
    // since this is an integer type,
    //   post-increment is okay here
    for (i = 0; i < v.size(); i++)
    {
        // ...
    }
    
share|improve this answer
    
Thanks Jamal again for your help, slightly confused about the data types mentioned will do some reading on the subject. –  ao2130 Oct 5 '13 at 3:41
    
@ao2130: Sounds good. I'd say the second point about size_type is more important. The unsigned int just looks nicer, but it shouldn't cause problems. –  Jamal Oct 5 '13 at 3:56
add comment

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