patterncppMinor
Prime generator
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:
And PrimeGenerator.h:
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
why don't you use
You don't need to use pointers in the PrimeGenerator class either.
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.