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

Prime generator

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

Problem

I'm coming from a Python background(look at indentation :) ) and trying to learn basics of C++. I wrote a simple prime generator, I know there is better algorithms, but I want advices about coding style, inconsistencies, or better language use.

Here is my main.cpp:

#include  
#include "PrimeGenerator.h"

using namespace std;

int main(){
    PrimeGenerator *generator;
    generator = new PrimeGenerator();

    for(int i=0;; i++){
        generator->addPrime();
        coutgetPrimes()->back()<<endl;
        }

    delete generator; #I know this is unnecessary at the end of program, but I think it is a better practice.

    return 0;
    }


And PrimeGenerator.h:

#pragma once
#include 
#include 

typedef std::list PrimeList;

class PrimeGenerator{

    PrimeList *primes;

    public:

    PrimeGenerator();
    ~PrimeGenerator();

    PrimeList *getPrimes();
    void addPrime();
    };

PrimeGenerator::PrimeGenerator(){
    primes = new PrimeList;

    primes->push_back(2);
    primes->push_back(3);
    }

PrimeGenerator::~PrimeGenerator(){
    delete primes;
    }

void PrimeGenerator::addPrime(){

    unsigned long long int num = primes->back()+2;

    while(true){
        bool isPrime = true;

        for(auto it=primes->begin(); it!=primes->end(); ++it){

            if(*it > (num / 2)) 
                break;

            if((num % *it) == 0){
                isPrime = false;
                break;
                }
            }

        if(isPrime){
            primes->push_back(num);
            break;
            }

        num += 2;
        }
    }

PrimeList *PrimeGenerator::getPrimes(){
    return primes;
    }

Solution

You don't need to manage resources manually.
You are declaring a pointer to the new PrimeGenerator instance and releasing it at the end of the main function, but what happens if one of the calls inbetween (for example, addPrime()) fails? You end up with a memory leak.

Instead of

PrimeGenerator *generator;
generator = new PrimeGenerator();
....
delete generator;


why don't you use

PrimeGenerator generator;
....


You don't need to use pointers in the PrimeGenerator class either.

class PrimeGenerator
{

        PrimeList primes;

    public:
        PrimeGenerator();

        const PrimeList& getPrimes() const {return primes;}
        void addPrime();     
};

std::ostream& operator<<(std::ostream& stream, PrimeList const& value)
{
    for (PrimeList::const_iterator it = value.begin(); it != value.end; it++)
    {        
    stream << *it << endl;
    }

    return stream;
}

PrimeGenerator::PrimeGenerator()
{
    primes.push_back(2);
    primes.push_back(3);
}

int main()
{
     PrimeGenerator generator;

    for(int i = 0; i < 100; i++)
    {
         generator.addPrime();
    }
    cout << generator.getPrimes();

    return 0;
}

Code Snippets

PrimeGenerator *generator;
generator = new PrimeGenerator();
....
delete generator;
PrimeGenerator generator;
....
class PrimeGenerator
{

        PrimeList primes;

    public:
        PrimeGenerator();

        const PrimeList& getPrimes() const {return primes;}
        void addPrime();     
};

std::ostream& operator<<(std::ostream& stream, PrimeList const& value)
{
    for (PrimeList::const_iterator it = value.begin(); it != value.end; it++)
    {        
    stream << *it << endl;
    }

    return stream;
}

PrimeGenerator::PrimeGenerator()
{
    primes.push_back(2);
    primes.push_back(3);
}

int main()
{
     PrimeGenerator generator;

    for(int i = 0; i < 100; i++)
    {
         generator.addPrime();
    }
    cout << generator.getPrimes();

    return 0;
}

Context

StackExchange Code Review Q#4267, answer score: 6

Revisions (0)

No revisions yet.