1
\$\begingroup\$

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;
}
```
\$\endgroup\$
  • 1
    \$\begingroup\$ This code works for some inputs and fails for others (which OP may not have considered in his testing strategy). Therefore I think a vote-to-close would be harsh; but I also think that OP should have spent a bit of time cleaning up the code before posting, in consideration of others' time. OP: why do you think that the array out can get away with only 10 slots, given that there are (10 choose 2) = 45 possible pairs of inputs, and each pair consumes 2 slots in out? Also, why did you misspell Preforming [sic]? \$\endgroup\$ – Quuxplusone Feb 24 '20 at 21:58
  • \$\begingroup\$ Thank you for the input. Clearly programming, and well spelling too isn't quite my forte? sp. \$\endgroup\$ – matt snyder Feb 24 '20 at 22:10
  • \$\begingroup\$ I apologize for wasting anyone's time. And was unclear of where the best place to post was being that it was slightly working and a post I made previously on stackoverflow was directed to post here so I was just confused and I don't want anyone to be mad at me \$\endgroup\$ – matt snyder Feb 24 '20 at 22:11
  • \$\begingroup\$ i initially used 10 slots as the numbers i was using were 1 2 3 4 5 6 7 8 9 10 11 and didn't want to repeat print out \$\endgroup\$ – matt snyder Feb 24 '20 at 22:13
  • \$\begingroup\$ code was updated and now shouldn't give an error.. \$\endgroup\$ – matt snyder Feb 24 '20 at 23:36
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ I cannot thank you enough for taking the time out of your day to provide me with this feedback. This was an extra credit problem I was working on for a class I am taking I find myself spending far too much time focusing on the end result than the meat and potatoes of the program, so without a doubt I will take these critiques to heart. \$\endgroup\$ – matt snyder Feb 25 '20 at 1:56
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for this thorough breakdown I will use the information provided in an attempt to prevent future substandard practices. Much appreciated. \$\endgroup\$ – matt snyder Feb 25 '20 at 13:15
  • 1
    \$\begingroup\$ cout << "(" << out[index++] << " , " << out[index] << ")" has defined behavior since C++17 - LHS of << is sequenced before RHS (not saying it's good practice) \$\endgroup\$ – L. F. Feb 26 '20 at 10:49
  • \$\begingroup\$ @L.F. Interesting; g++ -std=c++17 (or -std=c++2a) still warns that operation on index may be undefined [-Wsequence-point]. \$\endgroup\$ – Toby Speight Feb 26 '20 at 10:58
  • \$\begingroup\$ @TobySpeight It's a bug in both GCC and Clang: stackoverflow.com/a/51785055/9716597 \$\endgroup\$ – L. F. Feb 26 '20 at 11:11

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.