Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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;

Is there any way I can improve this program making simple but at the same time more efficient? What I did was 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 my forgetting something? Can I improve it but keep it simple? I'm just a beginner testing what I have learned so far.

share|improve this question

migrated from stackoverflow.com Jul 18 '12 at 20:54

This question came from our site for professional and enthusiast programmers.

1  
use an array of int rather than array of int* would be a good start –  mathematician1975 Jul 18 '12 at 20:50
1  
Why use pointers?? –  Jesse Good Jul 18 '12 at 20:51
    
@JesseGood: Because he's a beginner and he should learn how this stuff works before jumping ship to the STL. –  Ed S. Jul 18 '12 at 20:51
2  
There were numerous similar "questions" in the last couple hours: stackoverflow.com/questions/11549727 stackoverflow.com/questions/11548571 Somebody really seems to insist on us doing their homework ;-) –  Philipp Jul 18 '12 at 21:40
3  
@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) –  David Rodríguez - dribeas Jul 18 '12 at 23:12

2 Answers 2

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.