### Constructive Criticism of Code

So, I finished polishing up some code I wrote... could I get some constructive criticism of it? Just want to be sure what I did in it is good practice.

 ``123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125`` ``````#include #include #include ///Prime Number Calculator class primes { std::vector primeNumbers; //the vector that will hold the prime numbers (not to be confused with the object which is singular) bool primechecker (int b); //checks for prime-ness void primedisplayer (int o); //displays the number sent to it, which should be prime (if not, something is horribly, horribly wrong) void setVector (int l); //assigns that new-found prime number to the vector that holds the prime numbers public: int numberPrimes; //the number of primes that the user wants bool memoryException = false; //whether the user's computer has enough memory to keep this running void calculatePrimes(); //the for loop that everything runs in primes (int); //the constructor }; primes::primes(int a) //constructor, for simplicity's sake { numberPrimes = (a-1); //assigns whatever value you send it to the number of primes you want } bool primes::primechecker(int b) { bool test = false; for(std::vector::iterator i = primeNumbers.begin(); i != primeNumbers.end(); i++) //iteratres through the vector of primes { if(b % *i == 0) { test = true; //if the number isn't prime, set test to true and exit the loop. break; //no need to check every single other number once you know it isn't prime } } return test; //return whether the number is prime or not } void primes::calculatePrimes() //the function where it all takes place { int m = 2; //make sure it starts checking primes at 2 as well primedisplayer(m); //this is so it starts off with 2 try //it's a vector, so exception-handling is mandatory { primeNumbers.emplace_back(2); //make the first prime number in the vector } catch(std::bad_alloc&) { std::cout << "Error: No memory to allocate initial value to vector. Exiting..."; memoryException = true; } if (memoryException == true) //if there is no memory to assign 2 to the vector numberPrimes = 0; //make it so the for loop will never run for(int i = 0; i < numberPrimes; i++) { while(primechecker(m) == true) m++; //as long as the number isn't prime, add 1 to it setVector(m); //when the number is prime, add it to the vector if(memoryException == true) //if an error occured in memory break; //exit out primedisplayer(m); //displays the current value of m } return; } void primes::setVector(int l) { try { primeNumbers.emplace_back(l); //add the prime number to the end of the vector of prime numbers } catch(std::bad_alloc&) { std::cout << "Error: Ran out of memory during allocation of new prime number to array."; memoryException = true; //set the error check to true if there's no memoroy left, so the program can exit } return; } void primes::primedisplayer(int o) { std::cout << o << std::endl; //displays the current value of m, which is the just-calculated prime number return; } int main() { int j; //number of prime numbers. std::cout << "Please enter the number of prime numbers you want, starting with 2. Enter 0 to\nexit.\n"; std::cin >> j; //get input std::cout << "\n"; //who doesn't like new lines? while(true) { if (j > 0) //if it's greater than 0, run the program { primes primeNumber(j); //create the object, calling the constructer with the inputted number (if valid) primeNumber.calculatePrimes(); //then calls the function to get the ball rolling break; //then exits the loop when complete } else if(j < 0) //if it's less than 0, ask again { std::cout << "Number is less than zero. Please try again.\n"; std::cin >> j; std::cout << "\n"; //with another complimentary new line (I mean really, can you ever go wrong with new lines?) } else //if it's zero, exit out (or if it's invalid input, like a letter) break; } return 0; }``````
Last edited on
If you caculate 1000 pime numbers and then ask to calculate 1001, it will calculate first 1000 all over again.
I suggest adding static vector of prime numbers and integer last checked number to your class
The code actually exits out beforehand, so you'd have to start it up again in the first place. The actual ability to keep going after whatever stopping point will be added later.
I mean:
 ``12345`` ``````primes prime1(1000); prime1.calculatePrimes(); primes prime2(1000); prime2.calculatePrimes(); //will made calculations again prime1.calculatePrimes(); //Will screw underlying vector ``````
 ``12345678910111213`` ``````void primes::setVector(int l) { try { primeNumbers.emplace_back(l); //add the prime number to the end of the vector of prime numbers } catch(std::bad_alloc&) { std::cout << "Error: Ran out of memory during allocation of new prime number to array."; memoryException = true; //set the error check to true if there's no memoroy left, so the program can exit } return; }``````

It would be better to let the exception propagate to the calling code, which would simplify your code and move the error handling where it should be: outside of the class. If kept as-is, you need to remove the ability for the calling code to arbitrarily set the memoryException member (and, on a related note, numberPrimes should also not be a public member.)

A better design might be to make your class model a sequence:

 ``12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061`` ``````#include #include class PrimeSequence { public: typedef unsigned value_type ; PrimeSequence() {} value_type operator()() ; private: bool _isCandidate(unsigned value) ; // as MiiNiPaa notes, one could make this static so that multiple sequences // aren't doing the same work. On the other hand, we could just make sure // we don't use more than one sequence object. std::vector _primes ; }; bool PrimeSequence::_isCandidate(unsigned value) { std::vector::iterator p = _primes.begin(); bool isCandidate = true ; while ( isCandidate && p != _primes.end() ) if ( !(value % *p++) ) isCandidate = false ; return isCandidate ; } PrimeSequence::value_type PrimeSequence::operator()() { if ( _primes.size() == 0 ) _primes.push_back(2) ; else { value_type candidate = _primes.back() == 2 ? 3 : _primes.back()+2 ; while ( !_isCandidate(candidate) ) candidate += 2; _primes.push_back(candidate) ; } return _primes.back() ; } int main() { std::cout << "Enter the number of primes you'd like to calculate: " ; std::size_t nPrimes ; std::cin >> nPrimes ; PrimeSequence pSeq ; for ( unsigned i=0; i
¿why is a `calculate()' function printing?
¿why are error messages send to standard output?

Also, use better identifiers.
Topic archived. No new replies allowed.