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:
- 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.
- 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.
- 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
- 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:
- 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
- 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.
delete
(ordelete[]
). There are better things to use suchstd::array
,int[]
,std::vector
, NOT arrays of pointers for something this simple... \$\endgroup\$