Премьер-генератора


Я возвращаюсь из мира Python(посмотрите на отступы :) ) и пытаюсь выучить основы c++. Я написал простую премьер-генератора, я знаю, что есть лучшие алгоритмы, но я хочу советы о стиле кодирования, нестыковки, или лучше язык использовать.

Вот мой main.cpp:

#include <iostream> 
#include "PrimeGenerator.h"

using namespace std;

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

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

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

    return 0;
    }

И PrimeGenerator.ч:

#pragma once
#include <list>
#include <iostream>

typedef std::list<unsigned long long int> 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;
    }


809
8
задан 20 августа 2011 в 07:08 Источник Поделиться
Комментарии
4 ответа

Ваш стиль отступов необычно для C++. Существуют различные стили, используемые в C++, я рекомендую вам взглянуть на них, http://en.wikipedia.org/wiki/Indent_styleвыбери тот, который вы любите. Как это вы не расчесывал элементы нескольких стилей. Нам не нужно больше стилей отступов, так что используйте стандартный, не придумывать свои собственные.

#include <iostream> 
#include "PrimeGenerator.h"

using namespace std;

Многие люди используют это, но я рекомендую против него. Похоже делают из СТД импорт * в Python. Я думаю, что его лучше всего использовать префикс.

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

Вы можете объединить это в одну строку:

PrimeGenerator * generator = new PrimeGenerator();

Вы также можете избежать, используя указатель и написать:

PrimeGenerator generator;

for(int i=0;; i++){

Я думаю, что вам не хватает среднего элемента, так как он это бесконечный цикл

        generator->addPrime();
cout<<generator->getPrimes()->back()<<endl;

Я бы добавить пробелы вокруг <<

        }

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

Если вы используете pointerless версию я предложил, он будет автоматически уничтожен по окончании основной.

    return 0;
}

#pragma once

Это может не работать на всех компиляторах, так что будьте осторожны.

#include <list>
#include <iostream>

typedef std::list<unsigned long long int> PrimeList;

Список-это связанный список, в отличие от списков в Python, которые являются изменяемыми массивами. Связанные списки-это почти всегда неправильный выбор как структура данных. Возможно, вектор будет лучше? Акт векторы, такие как списки в Python. беззнаковый длинный интервал может быть записан как неподписанные долго долго. Вы также повторять это достаточно часто, создавая на typedef может быть хорошей идеей.

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;
}
}

Ваша логика может быть упрощена:

bool isPrime = false;
unsigned long long int num = primes->back();
while(!isPrime)
{
num += 2;

for(auto it = primes->begin(); it != primes->end(); it++)
{
if(*it > num / 2)
{
break;
}

if( num % *it == 0 )
{
isPrime = false;
break;
}
}
}
primes->push_back(num);

Я пытаюсь избежать, используя перерыв, когда это возможно. В основном, я стараюсь ограничивать в тех случаях, когда он пропускает остальные петли, как оптимизация. Если перерывы были удалены, код будет действителен, но медленнее.

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

6
ответ дан 20 августа 2011 в 10:08 Источник Поделиться

Вам не нужно управлять ресурсами вручную.
Вы объявляете указатель на новый экземпляр PrimeGenerator и выпускать его в конце функции main, но что произойдет, если один из звонков между ними (например, addPrime()) не? Вы в конечном итоге с утечкой памяти.

Вместо

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

почему вы не используете

PrimeGenerator generator;
....

Вам не нужно использовать указатели в классе PrimeGenerator либо.

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;
}

6
ответ дан 20 августа 2011 в 10:08 Источник Поделиться

Мне кажется, что премьер номер поколения действительно лучше представить как алгоритм. Вместо того, чтобы дать клиенту доступ к внутренностям класса-я бы тоже предпочел отправить результаты на итератор. Наконец, я бы предпочел, чтобы производить набор результатов производства одним из результатов одновременно.

Как таковой, кажется, что интерфейс должен быть что-то вроде:

template <class FwdIt>
void gen_primes(size_t num, FwdIt out);

Если вы хотите, например, чтобы произвести первые 100 простых чисел, вы бы назвали это что-то вроде этого:

std::vector<int> primes(100);

gen_primes(100, primes.begin());

Обратите внимание, что, поскольку он использует сведения в выходных данных, это требует вперед итератор, а не итератор вывода, так что вы не могли (например) использовать с std::back_inserter(праймы).

Хотя я бы, наверное, учесть большинство (например) @Уинстон Эверт предложениями, внутри код может остаться по крайней мере достаточно близко к тому, что у вас уже есть.

2
ответ дан 22 августа 2011 в 03:08 Источник Поделиться

Я думаю, что объектно-ориентированный дизайн может быть улучшен.


  • PrimeGenerator - это инструмент для создания связанного списка. Как только вы возвращаете указатель на него, кто знает, что звонящий будет делать с ее содержимым? Абонент может изменить список таким образом, что перерывы последующие звонки .addPrime().

  • Простые числа представляют собой условно бесконечный список, так что ваш класс должен попытаться создать иллюзию бесконечного списка. Требуя от пользователей, чтобы позвонить .addPrime() - это муторно, что нарушает эту иллюзию.

Было бы лучше, если PrimeList были самоуправляемые только для чтения список простых чисел, например:

class PrimeList {
public:
PrimeList();

// Returns the nth prime number
// (primelist[0] is 2, primelist[1] is 3, etc.)
unsigned long long operator[](size_t nth);

private:
std::vector<unsigned long long> knownPrimes;

void findMorePrimes();
};

Реализация функции operator[] может вызвать findMorePrimes() по мере необходимости за кулисами.

Понятие бесконечного списка также освобождает вас от ожиданий, которые вы должны добавить только одно простое число в списке. Затем, вы бы гибкость, чтобы найти пакеты простых чисел одновременно, с вероятностью улучшений в производительности.

Отвечать на плагиат с вот.

2
ответ дан 20 декабря 2013 в 01:12 Источник Поделиться