5
\$\begingroup\$

I am new to C++ and this is my first program. The objective of this program is to fully complete the sequence called the Collatz conjecture.

It concerns a sequence defined as follows: start with any positive integer n. Then each term is obtained from the previous term as follows: if the previous term is even, the next term is one half the previous term. Otherwise, the next term is 3 times the previous term plus 1. The conjecture is that no matter what value of n, the sequence will always reach 1.

I am not familiar with the language just yet and would appreciate some (constructive) feedback. The program works as intended, however, I have a feeling that some of my code is not as elegant as it could/should be.

#include <iostream>
#include <ctime>

using namespace std;

bool isValid(int num) {
  if(num <= 0) {
    return false;
  } else {
    return true;
  }
}

bool isEven(int num) {
  if (num % 2 == 0) {
    return true;
  } else {
    return false;
  }
}

void calculate(int num) {
  if (num == 1) {
    cout << endl << "Finished!" << endl;
    return;
  } else if(isEven(num)) {
    num = num / 2;
    cout << num << " ";
    calculate(num);
  } else {
    num = (num * 3) + 1;
    cout << num << " ";
    calculate(num);
  }
}

int main() {

  int input;
  cout << "Please enter an integer greater than zero." << endl;
  cin >> input;

  if(isValid(input)) {
    calculate(input);
  } else {
    cout << "Not a valid input." << endl;
    return 0;
  }

  return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Has anyone tried running this with 715827883 or higher? :) \$\endgroup\$ – Brian Sutherland Sep 12 '17 at 22:48
  • 2
    \$\begingroup\$ 113383 is also apparently a very interesting number to run this program with \$\endgroup\$ – Brian Sutherland Sep 12 '17 at 22:51
11
\$\begingroup\$

Good job checking the input for validity and breaking up the logic into simple functions! However, some improvements can be made.

Avoid using namespace std

This can cause name collisions because it adds every name in the std namespace to the global namespace. For a small program like this one it's unlikely that you'll run into any problems (then again, maybe not) but it's best to get into the habit of using the std:: prefix on names in the std namespace.

Alternatively, you can introduce using declarations like using std::cout; to add specific names to the global namespace.

Avoid std::endl in favor of \n

std::endl flushes the stream, which can cause a loss in performance. In some cases you can also reduce calls to operator<< (e.g. cout << endl << "Finished!" << endl; becomes cout << "\nFinished!\n";).

Simplify your boolean expressions

You can remove some of your explicit return true and return false statements by returning the expression in the if condition. For example, you can shorten isEven() like so:

bool isEven(int num) {
  return num % 2 == 0;
}

Similarly, you can shorten isValid().

Don't mix logic and writing to cout in calculate()

Currently calculate() is not very reusable since it writes to cout. What if you wanted to write to a different stream, or if you just wanted to calculate the next term in the sequence without writing to any stream? I suggest redefining calculate() to simply return the next term in the sequence:

int next(int prev) {
  if (isEven(prev))
    return prev / 2;
  else
    return prev * 3 + 1;
}

Then in main() you can output each number returned by next() to cout, as well as check for the condition in which the term equals 1. You would replace the calculate(input); statement in main() with something like this:

while (input != 1) {
  input = next(input);
  std::cout << input << " ";
}

std::cout << "\nFinished!\n"
\$\endgroup\$
  • 2
    \$\begingroup\$ I fixed the next function to not rely on a global variable, and the loop to terminate. I hope it matches your style. \$\endgroup\$ – Roland Illig Sep 12 '17 at 6:47
  • \$\begingroup\$ Can't say I agree with the part about std::endl. If you want to see your output or logs as they happen, its essential to use endl. It can be very confusing to debug a program that buffers its output. It's good to know the implications of flushing the buffer, but if you're at the point where you're using cout to see your program flow, you need it to be as responsive as possible. \$\endgroup\$ – JPhi1618 Sep 12 '17 at 14:37
  • \$\begingroup\$ However, I do agree that the endl before "Finished!" is unnecessary and should be a \n. There's nothing to flush at that point. \$\endgroup\$ – JPhi1618 Sep 12 '17 at 14:49
  • \$\begingroup\$ @JPhi1618 The OP says the program works as intended, so I take that to mean he isn't debugging. And I don't see much point in flushing the stream for every term in the sequence just to see the output as it's generated. There's a time and a place for using std::endl to see the output as it is generated (I do it myself in certain situations) but I don't think this is one of them. \$\endgroup\$ – Null Sep 12 '17 at 14:57
  • \$\begingroup\$ Actually, I just noticed that the OP only flushes the stream at the end of the sequence and at a few other times (right before receiving the input and right before returning from main). It seems even more superfluous in this case since it would be flushed anyway. \$\endgroup\$ – Null Sep 12 '17 at 15:01
4
\$\begingroup\$

Your error handling is missing an important point. When you declare the variable input, it already has a value, though that value is typically unpredictable. Reading this value from the variable invokes undefined behavior (that's an official term), which would make your program useless.

The operator std::cin >> input is not guaranteed to write a value to the variable. It only does so if the input is valid. So when I type asdf instead of an integer, the old value is preserved (or in general, partly overwritten, but for an int, that's not possible).

After that, your code reads that undefined value. It must not do this.

To solve this problem, write all code like this:

int input;
if (std::cin >> input && input > 0) {
  std::cout << "The number is " << input << ".\n";
} else {
  std::cerr << "error: wrong input format\n";
  return 1;   // Returning 1 from the main function means an error.
}

The >> operator does not read the value of the variable, it only writes to it in the successful case. Therefore this program never reads the undefined value from the variable.

\$\endgroup\$
  • \$\begingroup\$ Shouldn't you be initializing the input variable in your example? \$\endgroup\$ – Frank Sep 12 '17 at 7:03
  • \$\begingroup\$ No. This usage is perfect since I'm only reading the variable after it has been definitely assigned a value. The >> operator also is a write-only operation on the variable. \$\endgroup\$ – Roland Illig Sep 12 '17 at 7:04
  • \$\begingroup\$ Yes, nervermind, I misread your code for some reason. \$\endgroup\$ – Frank Sep 12 '17 at 7:05
3
\$\begingroup\$

Adding to @Null's answer:

Very nice use (intentional or not) of tail recursion.

Use unsigned int when you want to represent numbers greater or equal to zero

unsigned int can represent twice as many numbers as int. It also gives the compiler more information about what your program does, which lets it do its job better.

Do not include headers you are not using

You are not using <ctime> for anything, so you should not include it.

Return something else than 0 in main when the program fails

The convention is that a program returns 0 on success, and something else on failure. It's generally just 1, but some programs encode the nature of the failure in that number.

else {
  cout << "Not a valid input." << endl;
  return 1;
}
\$\endgroup\$
2
\$\begingroup\$

If you are simply testing a boolean expression and returning true or false based on the result, you could just return the result of the boolean expression.

Rather than

if (num % 2 == 0) {
  return true;
} else {
  return false;
}

You could simply have

return num % 2 == 0;

since the result of num % 2 ==0 already is a bool.

To do this with your isValid function, you would have to either negate the boolean expression, or change the way you are comparing the numbers.

The expression num <= 0 would become !(num <= 0), which might be a little more confusing, or simply change it to num > 0.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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