patterncppModerate
Find the perfect numbers within the range of 1 and a chosen number
Viewed 0 times
thenumberrangenumbersperfectwithinchosenfindand
Problem
My program is complete and working, but I would like second opinions on it. The program prompts the user for a positive value that will serve as the maximum for the range that will be checked for perfect numbers. Although this is one of those do-what-you-want-with-it problems, I have to keep the two prototypes. Are there any extra adjustments I should make in the program or is it already efficient the way it is?
#include "stdafx.h"
#include
using namespace std;
/* Function prototypes */
bool isPerfect(int n);
int sumOfProperDivisors(int n);
/* Main program*/
int main()
{
// Declare variable(s)
int bound; // Stores upper limit of number range
// Prompt user to enter a positive integer
cout > bound;
// Display the information
cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;
for (int i = 1; i <= bound; i++)
{
if (isPerfect(i))
{
cout << i << endl;
}
}
system("PAUSE");
return 0;
}
/*
* Function: isPerfect
* -------------------
* Returns the sum of the number(s) within the testing range
*/
bool isPerfect(int n)
{
int sum = 0;
for (int i = 1; i < n; i++)
{
if (n % i == 0)
{
sum += i;
}
}
return sum == n;
}
/*
* Function: int sumOfProperDivisors
* ---------------------------------
* Returns the sum of a number's divisors
*/
int sumOfProperDivisors(int n)
{
int sum = 1;
for (int i = 2; i <= n/2; i++)
{
if ((n % i) == 0)
{
sum += i;
if (i * i == n)
{
sum -= i;
}
}
}
return sum;
}Solution
MS Specific
We don't need this MS-specific header:
Namespace std
Never do this:
I know every book you read does this. This is because they are paying for the ink. It's actually very bad style and will cause a huge amount of problems for any program longer than ten lines or for code that lives longer than a year. So it is a bad habit you should get out of the habit of doing.
The reason they picked
For details about the problem see: Why is “using namespace std;” considered bad practice?
Comments
Don't add useless comments.
Comments can do more harm than good when used incorrectly. This is because over time comments fall out of line with the code unless very explicitly maintained. Because the compiler does not check the comments for accuracy this happens a lot.
Thus a maintainer who sees a comment that does not agree with the code then has to try and work out which is wrong, the code or the comment, and then has to fix it.
You should restrict your comments to "WHY" comments. Why is the code working like this? "WHY" we need to do something? The code itself should explain "HOW" (as long as you use good variable and function names).
See: What are examples of comments that tell you why instead of how or what?
Useless comment: (I can see what it does by reading the code)
I don't like this comment:
In this case the comment says one thing. The name of the function says something else. It does not return the
I don't particularly like the comment:
It does not tell me anything except what the name of the function already tells me.
Validate User Input
You don't validate user input.
Input from many sources can be trusted. Input from a
std::endl and flushing
Prefer to use
The reason is that
see: C++: “std::endl” vs “\n”
Prefer Prefix increment
Prefer prefix increment
Though in this case it does not explicitly matter. If somebody came along and changed the type of your loop (to say
The reason that
see: Questions about operator overloading
The default method means that the suffix version creates an extra copy of the object that is not required by the prefix version.
System Pause Bad
Don't do this.
Use a standard happy way of pausing the the code.
Return in Main
The function
As a result we use having no return as an indicator to the maintainer that there is no way for this application to fail. If you have an explicit
We don't need this MS-specific header:
#include "stdafx.h"Namespace std
Never do this:
using namespace std;I know every book you read does this. This is because they are paying for the ink. It's actually very bad style and will cause a huge amount of problems for any program longer than ten lines or for code that lives longer than a year. So it is a bad habit you should get out of the habit of doing.
The reason they picked
std rather than standard so it is easy to type and use as a prefix. It's not that much harder to type than the normal object or type.std::cout << "Testing\n"; // Not that hard, see?For details about the problem see: Why is “using namespace std;” considered bad practice?
Comments
Don't add useless comments.
/* Main program*/
int main()
{
// Declare variable(s)Comments can do more harm than good when used incorrectly. This is because over time comments fall out of line with the code unless very explicitly maintained. Because the compiler does not check the comments for accuracy this happens a lot.
Thus a maintainer who sees a comment that does not agree with the code then has to try and work out which is wrong, the code or the comment, and then has to fix it.
You should restrict your comments to "WHY" comments. Why is the code working like this? "WHY" we need to do something? The code itself should explain "HOW" (as long as you use good variable and function names).
See: What are examples of comments that tell you why instead of how or what?
Useless comment: (I can see what it does by reading the code)
// Display the information
cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;I don't like this comment:
/*
* Function: isPerfect
* -------------------
* Returns the sum of the number(s) within the testing range
*/In this case the comment says one thing. The name of the function says something else. It does not return the
sum it returns bool. Because the function is named perfectly you don't really need to say anything (the function name is enough).bool isPerfect(int n)I don't particularly like the comment:
/*
* Function: int sumOfProperDivisors
* ---------------------------------
* Returns the sum of a number's divisors
*/It does not tell me anything except what the name of the function already tells me.
Validate User Input
You don't validate user input.
cin >> bound;Input from many sources can be trusted. Input from a
human can not. Validate their input and anticipate that they will either be stupid or malicious in trying to crash your program.std::endl and flushing
Prefer to use
"\n" instead of std::endl.cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;The reason is that
std::endl just puts "\n" then flushes the stream. Stream flushing can make the code very inefficient if done incorrectly. Also the streams flush themselves when they need to without your help.see: C++: “std::endl” vs “\n”
Prefer Prefix increment
Prefer prefix increment
++i over suffix increment i++.for (int i = 1; i <= bound; i++)Though in this case it does not explicitly matter. If somebody came along and changed the type of your loop (to say
BigInt, a type that supports 128 bit integers) then it may make a difference. Also you want them to be able to change the type without having to change any code and still get the best performance.The reason that
i++ can be less efficient for types is because of the default way of implementing the operator.see: Questions about operator overloading
The default method means that the suffix version creates an extra copy of the object that is not required by the prefix version.
System Pause Bad
Don't do this.
system("PAUSE");Use a standard happy way of pausing the the code.
cin.ignore(255, '\n'); // ingore input to the end of line (as you did not do that in the previous read).
std::cout << "Hit Enter to continue\n";
cin.get(); // Wait for user input to be flushed.Return in Main
The function
main() is special. If you don't specify a return value the compiler will generate one for you - return 0. It is the only function this will happen in.As a result we use having no return as an indicator to the maintainer that there is no way for this application to fail. If you have an explicit
return 0; at the end then I am looking through your code to find a place where you return an error code.return 0;Code Snippets
#include "stdafx.h"using namespace std;std::cout << "Testing\n"; // Not that hard, see?/* Main program*/
int main()
{
// Declare variable(s)// Display the information
cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;Context
StackExchange Code Review Q#101677, answer score: 19
Revisions (0)
No revisions yet.