-1

So basically I have to make an array that allows the user to randomly generate the amount of numbers of their choice, and none of the numbers can be the same. My code generates them normally but still gets repeated numbers and I'm not sure why as I think I prevented it. I'm still fairly new to arrays so this may look really dumb, any help would be appreciated! I've left a lot of side notes to try and break down each section, and at the bottom I'll include what it looks like when it runs.

#include <iostream>
#include <ctime>
#include <windows.h>
using namespace std;

int main()
{
    int numberlist[20]; //the array
    int count=0,amount=0,value=0;
    bool found=false;

    srand(time(NULL)); //makes randomizer not the same

    cout << "How many numbers to generate?" << endl;
    cin>>amount; //gets user input

    for(int count=0; count<amount; count++){
    value = rand() % 40 + 1; //generates random number from 1-40 until amount is reached
    found=false;

    if(value==numberlist[count])found=true; //if value is in the array, change found from false to true

    if(value!=numberlist[count] && found==false){ //if number is unique and new
    numberlist[count]=value;                   //add number to array
    cout << value << endl; //show the value to the screen
    }
    else if (found==true) {count--;} //if value is in array erase 1 from count

    }
    return 0;
}

//What it looks like altogether
//How many numbers to generate?
// 9 (<the users input)
//37
//5
//30
//13
//7
//18
//1
//25
//25 (The 25 is the repeating number in this case)
namikaze123
  • 1
  • 1
  • Note if `amount > 40` duplication is assured – user4581301 Dec 09 '20 at 01:03
  • 3
    Here's an idea: Make an array of 40 elements. fill it with 1 to 40. [Shuffle the array.](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle). Pick out the first `amount` items from the array. – user4581301 Dec 09 '20 at 01:05
  • The example at the bottom of [this documentation page for `std::iota`](https://en.cppreference.com/w/cpp/algorithm/iota) shows you exactly how to leverage the C++ Standard Library to do all of the work for you. – user4581301 Dec 09 '20 at 01:07
  • If you know the number of values to be generated, generate that number of unique values (e.g. into an array or other container) by whatever means is appropriate. Then randomly shuffle that container, and draw values in order. It is normal for random number generators to give duplicate values - "generate random value" and "avoid duplication" are, to some extent, contradictory requirements. – Peter Dec 09 '20 at 01:11
  • Tip: Instead of `rand()`, which has severe limitations, consider using [`std::uniform_int_distribution`](https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution) which produces high-quality random results without all the caveats. – tadman Dec 09 '20 at 01:15
  • Tip: Instead of `if (x == true)` you can do just `if (x)`. This makes your code a lot less cluttered and easier to follow. – tadman Dec 09 '20 at 01:16
  • Another option is generate the number and add to [std::unordered_set](https://en.cppreference.com/w/cpp/container/unordered_set/find) -- though shuffling an array is probably more efficient. Fisher-Yates Shuffle. – David C. Rankin Dec 09 '20 at 01:18
  • Related: https://stackoverflow.com/questions/7114043/random-number-generation-in-c11-how-to-generate-how-does-it-work https://stackoverflow.com/questions/13445688/how-to-generate-a-random-number-in-c – Jerry Jeremiah Dec 09 '20 at 01:21
  • You're right std functions would be more efficient but I'm still a beginner and trying to learn the basics first, but thank you for the help! – namikaze123 Dec 09 '20 at 01:46
  • Here is a sample of random integers from 1 to 40 with no duplicates bu the shuffling an array: https://onlinegdb.com/OxhZOoir2 – Jerry Jeremiah Dec 09 '20 at 01:47
  • @tadman - One of the caveats that still applies to `std::uniform_int_distribution` (at least, for this question) is that it is not guaranteed to give a set of unique values. – Peter Dec 09 '20 at 01:53
  • @Peter No, but you can at least throw out values you've already seen. – tadman Dec 09 '20 at 02:07
  • 1
    @namikaze123 - If you want to "learn the basics", there is value in reading documentation for the standard algorithms - because, in practice, some sites (like en.cppreference.com ) provide sample implementations of the algorithms. Doesn't change the basic fact that your approach (generate random numbers, throw out duplicates until you have enough) is not guaranteed to work (e.g. if you need the values within a bounded time, you may need to terminate the loop before enough values are produced). Whereas, producing a set of unique values and shuffling (using a rng) avoids such concerns entirely. – Peter Dec 09 '20 at 03:22
  • @Peter I see, thats good to know for future use. I'll definitely check out the documentations and try to add more to my knowledge to figure this out, thank you for the information! – namikaze123 Dec 09 '20 at 15:56

1 Answers1

0

you have mistake in your logic

if(value==numberlist[count])found=true; //if value is in the array, change found from false to true That's only check if there is no duplicates on in position equals count, You have to iterate over all array position!

Full code below:

#include <iostream>
#include <ctime>

using namespace std;

int main()
{
    int numberlist[20]; //the array
    int count=0,amount=0,value=0;
    bool found=false;

    srand(time(NULL)); //makes randomizer not the same

    cout << "How many numbers to generate?" << endl;
    cin>>amount; //gets user input

    for(int count=0; count<amount; count++){ 
    value = rand() % 40 + 1; //generates random number from 1-40 until amount is reached
    found=false;

    for(int i = 0 ; i < count; i++) // iterate over all position in array
    if(value==numberlist[i])found=true; //if value is in the array, change found from false to true

    if(found==false){ //if number is unique and new <--- I have also fix this condition
    numberlist[count]=value;                   //add number to array
    cout << value << endl; //show the value to the screen
    }
    else if (found==true) {count--;} //if value is in array erase 1 from count

    }
    return 0;
}
Kreans
  • 60
  • 7
  • Ofc, I have only fix your logic. Probably You have just started learning programming and you need the most simple solution with no sets or something like that( which could be more efficient) – Kreans Dec 09 '20 at 01:24
  • You can provide two variants of the code, one simple and one more elegant, though. – Aziuth Dec 09 '20 at 01:40
  • Ahh I see, in the bottom I use count-- to remove the latest repeat so it doesn't actually remove the number? So I how would I remove the number itself, would numberlist[count]-1 work? Thank you for the help! – namikaze123 Dec 09 '20 at 01:45
  • Here you are, There is a quite big examples of this https://codereview.stackexchange.com/questions/61338/generate-random-numbers-without-repetitions – Kreans Dec 09 '20 at 01:51
  • You don't have to remove the number. It's enough to not put it into the array :). Your for loop increment count every time, thanks to count--; you are picking good number of integers(in case if value is already in array). I recommend to use while loop instead of for loop and increment a count only if found==false – Kreans Dec 09 '20 at 01:58
  • After reading your code again I see what you did! If i'm reading it correctly, when the value is a repeat you put it in numberlist[i] so that numberlist[count] has all unique numbers. Again thanks 1000x over!! – namikaze123 Dec 09 '20 at 02:17