2
\$\begingroup\$

I prompt the user for 9 numbers, store those numbers in 9 pointers and display the element 2. It seem to work just fine, but am I forgetting something? Can I improve it but keep it simple? I'm just a beginner testing what I have learned so far.

int n;
int *array[9];
bool isUsed[10] = {0};

for(int i = 0; i < 9; i++) 
{
    cout << "Enter Number " << (i + 1) << endl;
    cin >> n;
    if((n >= 0) && (n <= 9))
    {
        if (isUsed[n] == false)
        {
            array[i] = new int;
            *array[i] = n;
            isUsed[n] = true;
        }

        else
        {
            cout << "Number has already been used." << endl;
            i--;
        }
    }

    else 
    {
        cout << "Numbers from 0-9 only." << endl;
        i--;
    }

}

    cout << *array[2]<<endl;
\$\endgroup\$
11
  • 1
    \$\begingroup\$ use an array of int rather than array of int* would be a good start \$\endgroup\$ Jul 18, 2012 at 20:50
  • 1
    \$\begingroup\$ Why use pointers?? \$\endgroup\$
    – Jesse Good
    Jul 18, 2012 at 20:51
  • \$\begingroup\$ @JesseGood: Because he's a beginner and he should learn how this stuff works before jumping ship to the STL. \$\endgroup\$
    – Ed S.
    Jul 18, 2012 at 20:51
  • \$\begingroup\$ You have a memory leak if you forget to delete (or delete[]). There are better things to use such std::array, int[], std::vector, NOT arrays of pointers for something this simple... \$\endgroup\$
    – StackUnderflow
    Jul 18, 2012 at 20:51
  • 3
    \$\begingroup\$ @EdS.: I actually think otherwise: you should learn the high level C++ (i.e. standard libraries, managed memory ... ) before jumping into the gory details. You can write good C++ language without raw pointers (as a matter of fact, good C++ code avoids raw pointers) \$\endgroup\$ Jul 18, 2012 at 23:12

2 Answers 2

8
\$\begingroup\$

You can definitely improve it, by replacing the array of pointers with a std::vector<int>.

If you must track usage, I suggest using a map instead, where the keys are 0...9 and the values exist only if the respective index was added.

\$\endgroup\$
4
\$\begingroup\$

Firstly: 9 is a magic number, hardcoded in 4 places (if you include the isUsed[10] as 9+1). If you decide to go for 11 numbers instead, there's a decent chance you'll forgot to change one of those 4 locations, and it's entirely avoidable.

We'll switch to putting this in a single parameter (or single constant) so there's only one thing to change. Same for the magic number 2 ...

// permute integers 0..max
// return the position'th integer in the permuted set
int permute_and_select(int max, int position)
{
}

get your current functionality by calling permute_and_select(9,2).


Secondly, let's think about the algorithm a bit. You're asking the user for some permutation of [0..N], right? So our pseudo-code can be something like

for i in 1..N:
    get number
    if number < 1 or number > N: complain-and-repeat
    if number already used: complain-and-repeat
    record number used
    record number is i'th entry
return 2nd entry

notes about this sketch:

  1. I put the sanity checks at the start, because I think it's easier to read the successful path if you already know the preconditions. Otherwise I find myself reading it, and then trying to remember it while I check the else branch to see what happens there. Also, it reduces the depth of nesting which is a largely aesthetic preference.
  2. the complain-and-repeat stuff is done twice, and I'm not happy with the way it modifies the loop counter. This is fragile in the face of changes to the loop logic, since we now have to update the for and each of the two else branches together. We'll make this more robust.
  3. using a bool array for the number used logic is fine: we could use a bitset or something, but I don't see it as a problem
  4. using an array of int pointers for the values is probably over-complicated, here: we know how much storage is required in advance, so we can allocate it all up-front

Now, let's see how close the sample code is to the pseudo-code sketch ...

// permute integers 0..max
// return the position'th integer in the permuted set
int permute_and_select(int max, int position)
{
    // track which numbers we've already used
    bool *used = new bool [max+1];
    // OR std::vector<bool> used(max+1, false);
    // OR std::unique_ptr<bool[]> used(new bool [max+1]);

    int *permuted = new int [max];
    // OR vector, unique_ptr etc. as above

    for (int index = 0; index < max; /*omit increment*/)
    {
        int number; // we can declare variables where they're used
        cout << "Enter Number " << index+1 << endl;
        cin >> number;

        if (number < 0 || number > max) { // check bounds
            cout << "Numbers from 0-" << max << " only.\n";
            continue;
        }
        if (used[number]) { // check repetition
            cout << number " has already be used.\n";
            continue;
        }

        // we have a new, valid selection, so:
        // record number used
        used[number] = true;
        // record number is index'th entry
        permuted[index] = number;
        // only on successful input
        ++index;
    }

    int selection = permuted[position];

    // note that vector or unique_ptr take care of this automatically
    delete [] used;
    delete [] permuted;

    return selection;
}

There are a couple of gotcha's and niggles left in here:

  1. the used and permuted arrays are uninitialized. You can fix that with a loop or with memset, but it's another advantage of just using vector
  2. it's not clear from the question whether you want N+1 integers (covering [0,N] exactly) or N unique integers drawn from [0,N]. I've gone for the latter case, same as you, I'm just not certain it's what you meant.

The code's already pretty long for a sketch though, so I'm leaving them as an exercise for the reader.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.