Generate array with random numbers and search for an element in it

Question

This is a school project I've written. It works, but I feel it could be done better.

The program generate an array of 1'000 random numbers, and searches for a number typed into console, and displays the index if found.

int inp, zuf, i, num, trig=0;
const int amount = 1000;
int arr[amount];
cout << "Numbers" << endl << "======" << endl << endl;
cout << "Please type in a nuber between 1 and 1000: " << endl;
cin >> inp;
srand(time(NULL));
for (int i = 1; i < amount + 1; i++) {
    zuf = rand();
    zuf = (zuf % amount) + 1;
    arr[i] = zuf;
    num = i;

    if (inp== arr[num] && trig == 0) {
        cout << "The number exists at the position: ";
        trig++;
    }
    if (inp== arr[num]) {
        cout << num << ", ";
        num++;
        trig++;
    }
}

if (trig == 0) cout << "This number doesn't exist." << endl;
else cout << "\b\b." << endl;

Answer 1

Do not using using namespace std

bringing in entire namespaces can be convenient, but in the long run, it's a really bad habit.

You should explicitly use std::cout and the like instead.

Declare variables as close as possible to their first use.

int inp, zuf, i, num, trig=0;

This means very little out of context.

Prefer using std::array instead of regular c-style arrays.

std::array<int, amount> arr;

This plays better with the rest of the standard.

Do not use 1-based indexing unless you really need it.

And in your case, you definitely do not want it, as you are doing an out-of-bounds access on arr[i] when i == amount;

for (int i = 1; i < amount + 1; i++) {

should be

for (int i = 0; i < amount; i++) {

But it doesn't matter because:

prefer using ranged-based for unless necessary

for(auto& val : arr) {
  val = ...
}

Do not use srand()/rand()

They are terrible. It's slow and not particularly random. You should use the random number genreator provide by <random> instead:

https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution.

Separate your concerns:

I suspect you are not respecting the "spirit of the exercise", since filling the array doesn't serve a purpose in your example.

You probably want to first fill the array, and then ask the user (possibly multiple times) to provide a number to find.

Answer 2

srand(time(NULL));

Forget that the legacy NULL macro ever existed. Don’t use it — ever.

const int amount = 1000;

constexpr is the new static const. You should write this as

constexpr int amount = 1000;

---

Your loop doesn’t make sense.

The first part is filling the array. But then you set num, and test the value you just populated, but then increment num so now it’s indexing part of the array you have not filled yet. Oh, but you ignore that as you set it back to i again. So what is num for? It looks like you combined a populate loop with a search loop and didn’t integrate it properly.

What is trig for?

amount is both the size of the array and the range of random numbers to use? Is that a coincidence?

I think you over-complicated the logic of printing out the results. Printing at the same time as finding is complicating the code, and not “normal” in terms of real application code.

---

Note that the searching should be done using std::find. And being interested in the index of the item in the array is abnormal and complicates easy use of both language features and library functions.

Fill the array:

for (auto& item : arr)
    item= get_my_rand();

There is no index in sight. You just get presented with each element of the collection in turn. Or, you could write

std::generate (std::begin(arr), std::end(arr), get_my_rand);

and that makes more sense if you used a range version of the algorithm, e.g.

boost::generate (arr, get_my_rand);

Now finding is done with

auto result = std::find (std::begin(arr), std::end(arr), inp);

and again that’s better with ranges, but the regular form above lets you find multiple instances by using the previous result, after incrementing, as the first parameter.

In either case, it does not return an index but an iterator, which you can use for other operations since everything takes iterators.

---

You can collect the positions of the found items in a vector and return it all at once. Then, print them. The printing routine knows in advance if the vector is empty, singular, or plural.

Printing with comma separators is harder that it ought to be, as I lament in this article.

---

You should read through and bookmark the C++ Standard Guidelines. You might find a lot different from the ancient text and bad habits you are being taught.