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

Object-oriented approach to sieve of Eratosthenes

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

Problem

I was trying to implement the sieve of Eratosthenes entirely by myself, using only what I have learned so far. Now I'm wondering what I can do to improve my code's readability. Optimizing the algorithm is not my goal for now.

#include 
#include 

using namespace std;

class num {
private:
    static unsigned obj_count; //simplifying initialization of the array, performance is not my main concern
    unsigned n;
    bool flagged;
public:
    num();
    void setn(unsigned);
    void setflagged(bool);
    bool isflagged();
    unsigned getn();
};

void Eratosthenes_sieve(num*, unsigned);

//Initialization of static member
unsigned num::obj_count = 0;

//Implementation of member functions
num::num() {
    flagged = false;
    ++obj_count; //simply declaring an array of class num will result in initialized values for n
    n = obj_count;
}

void num::setn(unsigned n) {
    this->n = n;
}

void num::setflagged(bool flagged) {
    this->flagged = flagged;
}

bool num::isflagged() {
    return flagged;
}

unsigned num::getn() {
    return n;
}

int main()
{
    unsigned n = 100;
    num arr[n];

    Eratosthenes_sieve(arr, n);

    return 0;
}

void Eratosthenes_sieve(num *arr, unsigned n) {
    unsigned i;

    for (i = 1; i < sqrt(n); ++i) { //minor optimization, as there will be no multiples or sqrt(arr[i]) that are lower than n
        if ( !(arr[i].isflagged()) ) { //if the number is not flagged, proceed to flag it
            for (unsigned j = 1; j < ( n / (arr[i].getn()) ); ++j) {
                arr[i + j * arr[i].getn()].setflagged(true); //flags every arr[i]-th number in the array.
            }
        }
    }

    cout << "Prime numbers up to " << n << ": \n";
    for (i = 1; i < n; ++i) {
        if ( !(arr[i].isflagged()) ) {
            cout << arr[i].getn() << " ";
        }
    }
}

Solution

Is "n" really a good idea?

I don't like arr[i].getn(). The whole algorithm relies on the fact that arr[i].getn() is exactly i+1. If this is ever not true, then the sieve will not work. Given that the array index is inextricably linked to the value of n, I wouldn't even bother with n at all.

Using constant expressions in loop conditions

In two places, you use constant expressions in your loop condition:

for (i = 1; i < sqrt(n); ++i) // sqrt() on every loop
for (unsigned j = 1; j < ( n / (arr[i].getn()) ); ++j) // division on every loop


What's bad about this is that you will be calling sqrt() or doing a division on every loop, even though the value of the expression will never change. It's possible the compiler could optimize this for you, but it's not a certainty. It's easy enough to just move the expression out of the loop yourself:

int sqrtN = sqrt(n);
for (i = 1; i < sqrtN; ++i)


Simplifying the loop

Even after getting rid of getn(), the second loop can still be simplified. The current loop is this:

for (unsigned j = 1; j < (n / (i+1)); ++j) {
    arr[i + j * (i+1)].setflagged(true);
}


It can be simplified to this:

for (unsigned j = i+(i+1); j < n; j += (i+1)) {
    arr[j].setflagged(true);
}


In other words, instead of computing the array index using a multiplication, you can just have j be the array index. This way avoids any multiplications and divisions and in my opinion is also simpler to read. It would be even simpler if arr[i] stood for the number i instead of i+1.

Code Snippets

for (i = 1; i < sqrt(n); ++i) // sqrt() on every loop
for (unsigned j = 1; j < ( n / (arr[i].getn()) ); ++j) // division on every loop
int sqrtN = sqrt(n);
for (i = 1; i < sqrtN; ++i)
for (unsigned j = 1; j < (n / (i+1)); ++j) {
    arr[i + j * (i+1)].setflagged(true);
}
for (unsigned j = i+(i+1); j < n; j += (i+1)) {
    arr[j].setflagged(true);
}

Context

StackExchange Code Review Q#92414, answer score: 3

Revisions (0)

No revisions yet.