Calculating the number of primes between 3 and another number

Question

This is a simple program that gets a number from a user and calculates the number of primes between 3 and the user's number. I'm just curious if I'm using too many variables. I'm only asking since I'm used to not needing to initialize iterator variables before a loop.

#include <iostream>
using namespace std;
int isPrime(int);
int main() {
    int userValue;
    int b;
    int x;
    char c;
    cout<< "Enter a number : "<< flush;
    cin >> userValue;
    for (x = 3; x <= userValue; x= x+2){
        a = isPrime(x);
        if (a == 1){ 
        b +=1;    
        }
    }
    cout << "There is, " << b << " number of primes from 3 to "<< userValue << endl;    
    cout << "hit enter to terminate..."<< flush;
    cin.get();
    cin.get();    
}

int isPrime(int number){
    int i;
    for(i=2; i<number; i++){
    if (number % i == 0 && i != number) return 0;
    }
    return 1;
}

Answer 1

No, I wouldn't say you're using too many variables (except for c, which doesn't appear to be used anywhere). Not enough functions, though. And perhaps too many spurious assignments. Specifically, I'd suggest something like:

#include <iostream>
using namespace std;

bool isPrime(int);
int countPrimes(int);
int main() {
    int userValue;
    cout<< "Enter a number : "<< flush;
    cin >> userValue;

    cout << "There are, " << countPrimes(userValue) << " number of primes from 3 to "<< userValue << endl;    
    cout << "hit enter to terminate..."<< flush;
    cin.get();
    cin.get();    
}

int countPrimes(int number) {
    int result = 0;
    for (int x = 3; x <= number; x += 2){
        if (isPrime(x)){ 
            result++;    
        }
    }
    return result;
}

bool isPrime(int number){
    for(int i=2; i<number; i++){
        if (number % i == 0 && i != number) return false;
    }
    return true;
}

That will do the same thing as your code. Also, please consider using variable names that are not just single-letters.

Answer 2

• Do you really need std::flush after each output? It does take some extra time, but fortunately it's not being used too often in this program.

• This does look like a C program, especially with isPrime() returning an int instead of a bool. You would then either return true or false instead of 1 or 0. I see that this has already been mentioned by @aroth, but I wanted to make sure you were clear on that.

• Some of your indentation is consistent, particular inside the for loops.

• std::cin.get() technically extracts a character from the screen, not waits specifically for the user to press enter.

  You could instead say something more accurate like this:

  "Enter any key and press ENTER to terminate"

• The x= x+2 can be shortened to x += 2. This is also a bit easier to read.

Answer 3

Yes, you are using too many variables: the compiler should have told you that c was unused, and you should always compile with warnings turned on.

However, it is hard to keep track of them when so many of them have one-letter names. It would be more pleasant if you renamed b → primeCount, x → primeCandidate.

In addition, I would extract the loop into its own primeCount() function for clarity and reusability.

In isPrime(), the i != number condition is redundant, since the loop-terminating condition is i < number. Actually, you could terminate earlier, at ceil(sqrt(n)). However, a much more efficient way for testing the primality of many numbers is to use another algorithm, such as the sieve-of-eratosthenes.