HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMajor

Prime number generator in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberprimegenerator

Problem

I am slowly learning C and C++, and decided after a few lessons to dive in myself without any help. I developed this prime number generator. It seems fast, but I'm wondering if I'm following the best practices for C++, or if I am missing anything important.

#include "stdafx.h"
#include "stdio.h"
bool checkPrime(int Number);

int _tmain(int argc, _TCHAR* argv[])
{
    int currentNum;
    currentNum = 2;
    do {
        if (checkPrime(currentNum)) {
            printf("%d ", currentNum);
        }
        currentNum++;
    } while (1 == 1);
    return 0;
}

bool checkPrime(int Number){
    for (int a = 2; a < Number; a++){
        if (Number % a == 0) {
            return false;
        }
    }
    return true;
}

Solution

A few things in addition to what @Jamal and @200_success already wrote:

-
g++ on Mac OS X won't compile this code, because of #include "stdafx.h" and the int _tmain(int argc, _TCHAR* argv[]) signature. It's good to keep your code portable, unless you have a special reason not to. g++ can compile if you drop the stdafx.h import and if change the main declaration.

-
... and since you're not using the arguments of main, you could just declare without any args: int main() { ... }.

-
As you already used true in the checkPrime function, why not use it in the infinite while loop in main, instead of 1 == 1.

-
This may be a matter of taste, but I think while (true) { ... } is generally more readable and intuitive than do { ... } while (true).

-
... actually, as @200_success pointed out, a for (int currentNum = 2; ; currentNum++) { ... } loop would be even better: this way currentNum is declared in the block where it is used, eliminating potential side effects, and the counter is a natural element in a for loop. Notice the empty terminating condition, making this an infinite loop.

-
In checkPrime you named the variable int Number, but the common convention is to not capitalize variable names, use simply int number instead.

-
As @leetnightshade pointed out, place the opening curly either always on the same line as the function name ("Egyption brackets"), or always on the next line.

Suggested implementation

#include 

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

int main()
{
    for (int currentNum = 2; ; currentNum++) {
        if (isPrime(currentNum)) {
            std::cout << currentNum << " ";
        }
    }
}


This compiles with g++ without warnings and runs fine in Mac OS X and GNU/Linux. I would hope it works as expected in Windows too.

Code Snippets

#include <iostream>

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

int main()
{
    for (int currentNum = 2; ; currentNum++) {
        if (isPrime(currentNum)) {
            std::cout << currentNum << " ";
        }
    }
}

Context

StackExchange Code Review Q#46590, answer score: 31

Revisions (0)

No revisions yet.