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

Find the perfect numbers within the range of 1 and a chosen number

Submitted by: @import:stackexchange-codereview··
0
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:

#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.