5
\$\begingroup\$

I started learning C++ and the first exercise was to create a small program to add two numbers. No more requirements. I did this and put a small validate the user input. I am sure that it could be much better written, so I appreciate all the critics and suggestions.

#include <iostream>

int x;
int sum = 0;
int i = 0;

int addTwoNumbers()
{
    while(i<2)
    {
        std::cin >> x;
        if(std::cin.fail())
        {
           std::cout << "Error! Invalid Input" << std::endl;
           std::cin.clear();
           std::cin.ignore(256,'\n');
        }
        else
        {
             sum += x;
             i++;
        }
    }
    return sum;
}
int main()
{
    std::cout << "Enter two integers" << std::endl;
    return sum = addTwoNumbers();
}
\$\endgroup\$
  • \$\begingroup\$ I tried to learn before but didnt get much far. I saw a bit of python and c# \$\endgroup\$ – EAzevedo Sep 7 '17 at 19:48
  • \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. \$\endgroup\$ – Toby Speight Sep 8 '17 at 7:07
7
\$\begingroup\$

Good job on your first program!

Before moving on to the review, a quick note on using namespace std

As long as you do it in a cpp file (and not a header), bringing in symbols from a namespace one at a time is absolutely fine, since you are in control:

#include <iostream>

using std::cout;
using std::cin;
using std::endl;

Errors

main's return value

The main()'s return value is for program status, not program output. I suspect you meant to do:

cout << "the sum is: " << addTwoNumbers() << endl;
return 0;

Good practice improvements

Avoid globals

int x;
int sum = 0;
int i = 0;

These have no business being global variables, they belong inside of your functions, as close to their first use as possible:

for(int i=0; i<2;)
{
    int x;
    cin >> x;

istream::fail() vs operator bool()

Edit Disregard this recommendation. See the comment below.

instead of if(std::cin.fail()) you should simply do if(std::cin), it actually covers more scenarios as well.

istream::ignore()

Whenever you use an arbitrary number like 256, it's a sign that something is not how it should be. In this specific case, you want to do cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n')

From a higher level:

addTwoNumbers() does too many things

Try to keep your functions short, single-purpose and reusable.

As guidance, look at this version of your main().

int main() {
  cout << "Enter two integers" << endl;

  int first = getNumber();
  int second = getNumber();

  cout << "the sum is: " << (first + second) << endl;

  return 0;
}

getNumber() would now be be a usefull and reusable piece of code, which is what functions should be.

\$\endgroup\$
  • \$\begingroup\$ Using if (std::cin) together with a while loop can quickly lead to an endless loop. The original code carefully avoids this by only checking for conversion failure. \$\endgroup\$ – Roland Illig Sep 8 '17 at 5:38
  • \$\begingroup\$ @RolandIllig Thanks, I ammended the revierw. \$\endgroup\$ – Frank Sep 8 '17 at 5:44
2
\$\begingroup\$

The variables x, sum and i are currently global variables. But they are only used in the addTwoNumbers function. Therefore they should be declared inside that function. Just move the three lines down after the opening {.

The number 256 looks arbitrary to me. Instead of this number, you could read the rest of the line like this:

std::string line;
std::getline(std::cin, line);

Instead of std::cout << "some string" << std::endl, you can just write std::cout << "some string\n". This is faster than your current code.

You currently don't output the result of the calculation. You should do so like this:

int sum = addTwoNumbers();
std::cout << "The sum is " << sum << "\n";
\$\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.