principlecppMinor
Object-oriented approach to sieve of Eratosthenes
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
Using constant expressions in loop conditions
In two places, you use constant expressions in your loop condition:
What's bad about this is that you will be calling
Simplifying the loop
Even after getting rid of
It can be simplified to this:
In other words, instead of computing the array index using a multiplication, you can just have
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 loopWhat'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 loopint 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.