2-sum problem in C++

Question

I'm a beginner at C++; all my previous programming experience is mainly through PLCs in industrial maintenance.

I have to accept 10 integers and a target value, then print all the pairs that sum to the target.

The pairs cannot be of the same integer. For example, if the target value is 10 then we cannot have a pair of 5 + 5. This code basically executes well enough for me, however I get an overloaded int out at the end; I'm curious why that might be?

Also I understand there are much easier ways to accomplish this task. I feel pretty good with what I was able to come up with given the amount of time I have devoted so I certainly am fine with it, however I wouldn't mind seeing some examples how much more seasoned programmers would have attempted this, if anyone out there is bored.

#include<iostream>
#include<algorithm>
using namespace std;

int main()
{
    int run, a, b, ints[11], out[45];

    cout << "\n\t\tAfter inputting 11 integers, this program will evaluate\n\tthe first 10 integers, then will display the pairs of said integers\n\twhich when added together the total will equal the 11th integer.";
    cout << "\n\n\tPlease enter 11 integers: \n\n";

    for (run = 0; run <= 10; run++)
    {
        cout << "\t\t"; cin >> (ints[run]);
    }
    system("cls");
    run = 0;
    for (a = 0; a < 10 && run <= 10; a++)
    {
        for (b = 1; b < 10 && run <= 10; b++)
        {
            if (ints[10] == (ints[a] + ints[b]) && ints[a] != ints[b] && run <= 10)
            {
                out[run] = ints[a];
                run++;
                out[run] = ints[b];
                run++;
            }
        }
    }
    cout << "\n\t\tPerforming critical calculations . . .\n\t\t";
    system("pause");

    cout << "\n\t\t";
    if (*max_element(out, out + 10) == 0)
        cout << "No sum of integers found to equal desired total. ";
    else
    {
        cout << "Pairs of integers that equal the desired total are as follows; ";
        for (int index = 0; index < 11; index++)
        {
            if (out[index] > 0)
            {
                cout << "(" << out[index++] << " , " << out[index] << ")";
                if (index < 11)
                    cout << ", ";
                else if (index == 11)
                    break;
            }
                
        }
    }
    system("pause");
    return 0;
}

Answer 1

Algorithm:
It's brute-force, but for ten elements that's fine. However, I don't think it will work correctly - it's going to stop at five pairs regardless of how many were generated.

Also, the outer loop should stop before the last element and the inner loop should start one after the current value of the outer loop. Otherwise you're just making the same comparison multiple times.

The main thing I would suggest that there are other data structures besides basic arrays. In this case, a vector of pairs would work well.

std::vector<std::pair<int,int>> results;

...

results.push_back(std::make_pair(input[a], input[b]));

...

if(results.size() == 0) 
{
    std::cout << "No sum of integers found to equal desired total.\n";
}

...

for(int i = 0; i < results.size(); ++i) 
{
    std::cout << '(' << results[i].first << ", " << results[i].second << ')';

.
.
Style:
Always put brackets around your blocks, especially when there's a matching block that does have them (the if/else for showing the results).

Give variables meaningful names, and don't re-use them just because they're the same type. Also, you don't need to (and usually shouldn't) declare them all at the top of the method; declare them where they're used.

Separate the target number as its own variable, having it as the last item of the input array is less clear.

If you're not calling them anything more specific, i and j are the usual convention for loop variables.

The fake delay for "critical calculations" that in fact are already done is a bit silly. :P

Answer 2

I'll try and point out bad habits that might cause you problems in larger programs.

Firstly, avoid using namespace std;, especially at such broad scope. This pollutes the global namespace, greatly reducing the benefits of having namespaces at all. It's not much to type std:: where you need it, and/or you can selectively import a handful of identifiers into functions where you need them repeatedly.

We can split the functionality of that large main() into three main sections:

1. Get input values
2. Calculate the result
3. Print the result

The reason to do this isn't just to help us reason about each part separately (although that's a good thing). A bigger benefit comes when we want to test the function against several sets of inputs to ensure correctness (this is unit testing). With the monolithic main(), we'd need to construct a test harness that runs the whole program for each test case, which is much slower.

We can reduce the huge long line here:

cout << "\n\t\tAfter inputting 11 integers, this program will evaluate\n\tthe first 10 integers, then will display the pairs of said integers\n\twhich when added together the total will equal the 11th integer.";
cout << "\n\n\tPlease enter 11 integers: \n\n";

In C++, as in C, we can write string literals in pieces, and the compiler will assemble them into a single string:

std::cout << "\n\t\tAfter inputting 11 integers, this program will evaluate\n"
    "\tthe first 10 integers, then will display the pairs of said integers\n"
    "\twhich when added together the total will equal the 11th integer.\n";
std::cout << "\n\tPlease enter 11 integers: \n\n";

We need to be more careful about reading input here:

    cout << "\t\t"; cin >> (ints[run]);

Reading from a stream can fail (for example, if the user presents something that's not a number, or closes the stream). When that happens, the stream enters the "fail" state, and all subsequent reads will also fail, until the state is reset. For a simple program like this, we can probably get away with terminating the program when that happens:

    std::cout << "\t\t";
    std::cin >> ints[run];
    if (!std::cin) {
        std::cerr << "Failed to read input number\n";
        return 1;
    }

Similarly, std::system() may fail (as it does on this Debian system, where neither cls or pause are available programs). If the result of this call is non-zero, then we need to deal with the failure. Note also that we're missing the necessary include of <cstdlib>; we may fail to compile because of that. (Including <cstdlib> also gives us the useful EXIT_FAILURE macro, which we could use as return value in the error case above.)

We have a bug here:

            cout << "(" << out[index++] << " , " << out[index] << ")";

We both modify and use index here in the same statement, and the two uses are unsequenced relative to each other. That means it's indeterminate whether the second use gets the initial value of index or the incremented value. What we need to do is to separate these uses into two statements:

            std::cout << '(' << out[index++];
            std::cout << " , " << out[index] << ')';

That said, loops are easier to read if we don't modify the loop counter within the body; we could re-write it to advance by 2 each time:

    for (int index = 0;  index < 11;  index += 2)

            std::cout << '(' << out[index] << " , " << out[index+1] << ')';

This test is redundant:

            else if (index == 11)
                break;

The very next code to be executed here is the loop increment (index++) and test (index < 11), which will exit the loop in this case anyway.