# Simple C++ program to add two integers

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;

{
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;
}

• I tried to learn before but didnt get much far. I saw a bit of python and c# – EAzevedo Sep 7 '17 at 19:48
• 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?. – Toby Speight Sep 8 '17 at 7:07

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:

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.

• 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. – Roland Illig Sep 8 '17 at 5:38
• @RolandIllig Thanks, I ammended the revierw. – Frank Sep 8 '17 at 5:44

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";