I don't see anything particularly wrong with your code. Two things I do see though:
- By using the same seed, you'll always get the same list of numbers. Most people solve this by using the current time as a seed.
- Using the global constant MAXSIZE is probably bad style. It's fine for creating the array, but you should restructure your other functions to take an array and it's size as parameters, and then use that size value for your loop condition. This will be more adaptable to different sized arrays and especially useful if you deal with dynamic arrays.
Infarction
Posting Virtuoso
1,580 posts since May 2006
Reputation Points: 683
Solved Threads: 53
Some points:
• You need to tell us a bit more than the normal "this doesn't work" thing. What do you mean when you are not able to generate unique numbers? Is the array not filled completely or the numbers are getting repeated.
• You need to break out of the loop in your linear search if the required value is found.
• The loop counter in the loop which generates random numbers should not be incremented if a duplicate in found in the list.
• Use the current time as seed instead of a small number like 7 or 12.
~s.o.s~
Failure as a human
11,938 posts since Jun 2006
Reputation Points: 3,281
Solved Threads: 734
• You need to break out of the loop in your linear search if the required value is found.
He sets a boolean flag :icon_wink:
Infarction
Posting Virtuoso
1,580 posts since May 2006
Reputation Points: 683
Solved Threads: 53
Ah, I see it now. When I ran it with MAXSIZE of 20 on my machine that didn't come up. The problem is here:
void FillMainList(int list[])
{
int temp, test;
for (int i=0; i<MAXSIZE; i++)
{
temp = (rand() % 5000);
test = LinearSearch(list, temp);
if (test == -1) // if the new value isn't in the list
list[i] = temp; // then save it
// otherwise.... it'll skip this index and you'll have the uninitialized value
}
}
To fix that, you should put the random generator in a loop and use the LinearSearch results as a loop condition:
for(int i = 0; i < MAXSIZE; i++)
{
do {
temp = rand() % 5000; // get random number
} while (LinearSearch(list, temp) != -1); // repeat till it's unique
list[i] = temp; // save it
}
Infarction
Posting Virtuoso
1,580 posts since May 2006
Reputation Points: 683
Solved Threads: 53
Can't you just create an array of 5000 ints
for (int i =0; i< 5000; i++ )
{
array[i] =i;
}
Then shuffle them around. So you would be picking two random index positions and swapping them. Do this say a couple of hundred times.
Then just print out the first 200 elements and you will get 200 random elements with no repeats in a number range from 0 - 5000.
iamthwee
Posting Expert
5,950 posts since Aug 2005
Reputation Points: 1,543
Solved Threads: 439
... but I'm stuck on how to avoid filling it with duplicate values. I'm assuming that i'll have to use either a linear or binary search function while filling the array...
1. you cannot do a binary search; the array is not ordered.
2. a linear search is possible, but it is not very efficient. filling up an array of size N would takeO(N*N)
3. the smart thing to do would be to avoid the search alltogether.
to choose N unique numbers out of M
a. choose the first with probability N/M
b. choose the next with probability (N-1)/(M-1) if the first was chosen, N/(M-1) if it was not.
c. and so on.
this would take linear time. O(M)
here is an example of how this could be implemented.
#include <iostream>
#include <ctime>
#include <cstdlib>
#include <algorithm>
#include <iterator>
using namespace std ;
void fill_with_unique_rand( int* array, int N, int min_value, int max_value )
{
int available = max_value - min_value +1 ;
int required = N ;
for( int i=0 ; i<available ; ++i )
if( ( rand() % (available-i) ) < required ) // we have to choose required
// out of the remaining (available-i)
{
--required ;
array[required] = min_value + i ;
}
random_shuffle( array, array+N ) ; // if needed
}
template< int N > inline void fill_array_with_unique_rand( int (&array)[N],
int min_value, int max_value )
{ fill_with_unique_rand( array, N, min_value, max_value ) ; }
int main()
{
srand( unsigned( time(0)) ) ;
int a[10] ; fill_array_with_unique_rand( a, 101, 200 ) ;
copy( a, a+sizeof(a)/sizeof(*a), ostream_iterator<int>(cout," ") ) ;
cout << '\n' ;
int b[20] ; fill_array_with_unique_rand( b, 20, 40 ) ;
copy( b, b+sizeof(b)/sizeof(*b), ostream_iterator<int>(cout," ") ) ;
cout << '\n' ;
}
vijayan121
Posting Virtuoso
1,606 posts since Dec 2006
Reputation Points: 1,159
Solved Threads: 287
That has a very bad efficiency though. See my suggestion.
iamthwee
Posting Expert
5,950 posts since Aug 2005
Reputation Points: 1,543
Solved Threads: 439
Can't you just create an array of 5000 ints
for (int i =0; i< 5000; i++ )
{
array[i] =i;
}
Then shuffle them around. So you would be picking two random index positions and swapping them. Do this say a couple of hundred times.
Then just print out the first 200 elements and you will get 200 random elements with no repeats in a number range from 0 - 5000.
This would be time-efficient and space-inefficient.
vijayan121
Posting Virtuoso
1,606 posts since Dec 2006
Reputation Points: 1,159
Solved Threads: 287
iamthwee makes a good point. The fix I provided will only be moderately effective at best, with decreasing effectiveness as you try to build a longer list (since you have to search the list each time and have more chances of a duplicate).
Also, neither of these will work if you were to build a list over 5000, for obvious reasons. And by disallowing duplicates, the array is technically not random :icon_wink:
Infarction
Posting Virtuoso
1,580 posts since May 2006
Reputation Points: 683
Solved Threads: 53
>This would be time-efficient and space-inefficient.
Maybe, but for a newbie my way is easier to understand.
iamthwee
Posting Expert
5,950 posts since Aug 2005
Reputation Points: 1,543
Solved Threads: 439
To fix that, you should put the random generator in a loop and use the LinearSearch results as a loop condition:
for(int i = 0; i < MAXSIZE; i++)
{
do {
temp = rand() % 5000; // get random number
} while (LinearSearch(list, temp) != -1); // repeat till it's unique
list[i] = temp; // save it
}
• The loop counter in the loop which generates random numbers should not be incremented if a duplicate in found in the list.
;)
~s.o.s~
Failure as a human
11,938 posts since Jun 2006
Reputation Points: 3,281
Solved Threads: 734
> What does 'M' represent?
M is the cardinality of the set of numbers out of which we have to choose N *unique* numbers. (M>=N)
> Any chance you could give us more explanations?
a. probability that the first number is chosen is N/M
b. probability that the second number is chosen is (N-1)/(M-1) if the first was chosen, N/(M-1) if it was not.
ie. N/M * (N-1)/(M-1) + (M-N)/M * N/(M-1) == N/M
and so on.
here is a java transliteration of the alogorithm (caveat: java is a foreign language to me, there may be syntax errors. but the logic is sound).
public class unique_rand_generator
{
// fill up array with unique random integers
// from the range min_value .. max_value
// invariant: max_value - min_value + 1 (M)
// >= array.length (N)
public static void fill( int[] array, int min_value,
int max_value )
{
int available = max_value - min_value + 1 ;
int required = array.length ;
Random rng = new Random() ;
for( int i=0 ; i<available ; ++i )
{
// we have to choose required
// out of the remaining (available-i)
if( ( rng.nextInt(available-i) ) < required )
{
--required ;
array[required] = min_value + i ;
}
}
}
}
vijayan121
Posting Virtuoso
1,606 posts since Dec 2006
Reputation Points: 1,159
Solved Threads: 287