2
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You didn't ask for this but you could test if an integer is a prime in a much more efficient way. \$\endgroup\$ – SylvainD Feb 4 '13 at 2:35
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for catching the unused c variable. I converted this program from C program which needed a character variable for a scanf() function. \$\endgroup\$ – Austin Davis Feb 4 '13 at 4:21
2
\$\begingroup\$
  • 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.

\$\endgroup\$
2
\$\begingroup\$

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 bprimeCount, xprimeCandidate.

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 .

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.