-1

I've been coding in c++ for over a day now and I am currently stuck on this particular problem. I want to enter a integer and produce every prime numbers that are smaller than the given integer but the output always has random numbers that aren't prime. Here's the code:

#include <iostream>
#include <math.h>

using namespace std;

int check(int x){
    bool prime = true;
    if (x <= 1){
        prime = false;
        return 0;
    }
    for (int i = 2; i < x; i++){
        if (x % i == 0){
            prime = false;
            return 0;
        }
    }
    if (prime = true){
        return x;
    }
}

int main(){
    int r;
    cout << "Enter range: ";
    cin >> r;
    int primes[r];
    int n = 0;
    for (int i = 0; i < r; i++){
        if (check(i) != 0){
            primes[n] = i;
            n++;            
        }
        else{
        }
    }
    for (int i : primes){
        cout << i << endl;
    }
}

Help me fix this problem. Thanks.

ccrangers
  • 23
  • It's good that you included the full example code in the question, but could you please also include an example input, with the real output and the output you expect. – BoBTFish Jul 02 '21 at 07:47
  • 4
    `if (prime = true)` is an assignment, not a comparison – perivesta Jul 02 '21 at 07:48
  • @dave true, but the last block could as well be just `return x;`, because at that point `prime` is always `true` and `prime=true` evaluates to `true` – 463035818_is_not_an_ai Jul 02 '21 at 07:49
  • Added notes: Don't use `using namespace std`. VLAs are not supported in standard c++. And they are also somewhat dangerous in c. – HAL9000 Jul 02 '21 at 08:15
  • Technically, your program doesn't need the array `primes` at all. Just print the prime numbers as you find them. – HAL9000 Jul 02 '21 at 08:19
  • This doesn't address the question, but in `check` the variable `prime` doesn't really do anything. It can be removed and the function will still do exactly the same thing. `prime = false; return 0;` can be changed to simply `return 0;` in both places. And since the function returns immediately whenever it detects a non-prime value, if execution gets out of the `for` loop, the value is prime, so all that's needed after the loop is `return x;`. – Pete Becker Jul 02 '21 at 12:52

1 Answers1

3

The problem in your code is that you check numbers in the range [0,r] and assume to find r prime numbers in that range. Thats of course not correct. For example when in the range [0,r] there are p prime numbers then your output will print the p prime numbers and then r-p uninitialized values (thats undefined behavior, the output of your code could be anything).

Change the last loop to

for (int i=0; i<n; ++i) {
    cout << primes[i] << endl;
}

To see only entries of primes that you actually assigned a value in the loop before.

Also please consider that int primes[r]; is not standard C++. See here: Why aren't variable-length arrays part of the C++ standard?. Use std::vector for dynamically sized arrays. Actually in your code you do not need an array at all. You can simply print the prime numbers in the same loop where you check them:

if (check(i) != 0) { std::cout << i << "\n"; }

Moreover, your check is a little too complicated. You are using a flag that you don't need. The last if is wrong, because prime = true is assignment not comparison. However, that doesnt really matter because at that point prime is always true (if not you already returned), and prime=true does evaluate to true. The function can look like this:

int check(int x){
    if (x <= 1){
        return 0;
    }
    for (int i = 2; i < x; i++){
        if (x % i == 0){
            return 0;
        }
    }
    return x;
}

This will also prevent the compiler from issuuing a warning for not returning from all branches. Note that you only need to check factors <= sqrt(x) because when there is a factor bigger than that there must also be a factor smaller than that.

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185