Finding perfect numbers

Question

My teacher wants me to find all the perfect numbers between 2 and 1000 and must use:

• for loops
• % operator to determine if a number is a divisor
• static final variables for 2 and 1000
• A static method to return a boolean value by finding and adding the divisors of the integer parameter and comparing the sum to the parameter

The code works and I just wanted a second opinion on it.

public class Problem_B {
    //Search between 2 and 1000
    public static final int minNum = 2; 
    public static final int maxNum = 1000; 

    public static void main(String[] args) {
        for (int num=minNum; num <= maxNum; num++) 
        if (perfectNum(num)) {
            int i_sumDiv = 0;
            for (int j=1; j <= (num/2); j++) {
                if(num%j == 0)
                    i_sumDiv = i_sumDiv + j; 
            }
            if (i_sumDiv == num) 
                System.out.println(num + " is a perfect number.");
        }
    }

    public static Boolean perfectNum(int num) {
        // a variable that holds the sum of the factors 
        int sum = 0;

        for (int factor = 1; factor < num; factor++) {
            if (num % factor == 0) {
                sum += factor;
            }
        }

        if (sum == num) {
            return true;
        } else {
            return false;
        }
      }
    }

Answer 1

//Search between 2 and 1000
public static final int minNum = 2; 
public static final int maxNum = 1000; 

That comment doesn't say anything that the code doesn't already state IMO. Now if you need to change either value, you have to update the comment as well, otherwise you end up with this:

//Search between 2 and 1000
public static final int minNum = 2; 
public static final int maxNum = 10000; 

And would that be a typo in the code, or just a misleading comment?

for (int num=minNum; num <= maxNum; num++) 

Spacing isn't consistent between operators, that's distracting when you read it.

for (int num = minNum; num <= maxNum; num++) 

Reads more fluently. Now, you have a scope, but where's the opening brace? Code that's explicit won't omit them, even for a single statement.

if (perfectNum(num)) {

Might be I'm the verbose type, but I don't see a reason not to rename num to number or value, and perfectNum to isPerfectNumber, making the condition read like this:

if (isPerfectNumber(value)) {

Now, I like how minNum and maxNum are consistent with num, so I'd then rename them to minValue and maxValue to maintain that beautiful consistency.

int i_sumDiv = 0;

Now that's a bit of a mystery. why is there an i_ prefix? The Div abbreviation seems uncalled for, and makes the name a bit ambiguous.

Okay I need to stop the line-by-line analysis here.

---

You're doing the work twice. First here:

if (perfectNum(num)) {

and then if the function returns true, you do the work all over again - you know it's a perfect number already; you're ready to output right there and then:

if (isPerfectNumber(value)) {
    System.out.println(num + " is a perfect number.");
}

So let's look at that function.

public static Boolean perfectNum(int num) {
    // a variable that holds the sum of the factors 
    int sum = 0;

    for (int factor = 1; factor < num; factor++) {
        if (num % factor == 0) {
            sum += factor;
        }
    }

    if (sum == num) {
        return true;
    } else {
        return false;
    }
  }
}

I like factor much more than j. I'm seeing braces on all scopes, and although sum isn't a completely descriptive noun, it's much better than i_sumDiv: I like this implementation much better than the superfluous one that executes after this function returns.

The indentation is broken though:

    }
  }
}

Should be:

        }
    }
}

Also, this is redundant:

    if (sum == num) {
        return true;
    } else {
        return false;
    }

It can easily be replaced with a single return statement:

return sum == num;

for (int factor = 1; factor < num; factor++) {

That loop is inefficient; the (redundant) one in main is better.

Answer 2

As others have pointed out, your perfectNum() method has some problems. It is also inefficient, a big sin in computing. First note that factors come in pairs: x * y = num. Once you have found x, you have also found y. That gets you two for the price of one. Further notice that for any factor pair, (x, y), one is <= sqrt(num) and the other is >= sqrt(num). That cuts down on the number of factors you have to try.

For perfect numbers specifically, we have to treat the factor pair 1 * num = num differently, because 1 is counted in the sum of factors while the number itself is not. That means treating factor 1 differently. Easy enough as 1 is always a factor. Similarly, we have to treat square numbers differently. 8 * 8 = 64, but we only add 8 into the sum of factors once, not twice. That needs a tweak to the code to correct.

I have included some other suggested changes, such as variable names, and added some explanatory comments.

Math.sqrt() returns a double, and in some cases it will return something like: 6.99999999867923 for Math.sqrt(49). Converting that directly to an int gives 6, since the conversion truncates. Adding 0.5 ensures that the integer square root is rounded to the nearest integer.

This code still has a hidden fault, though it works for the question your teacher asked.

public static Boolean isPerfect(int num) {

    // 1 is always a factor, so we count it.
    int factorSum = 1;

    // Lower factor of a pair is always <= square root.
    // Add 0.5 for correct rounding of a double.
    int limit = (int)(0.5 + Math.sqrt(num));

    // Search from 2 up to sqrt(num) for low factor.
    for (int lowFactor = 2; lowFactor <= limit; lowFactor++) {
        if (num % lowFactor == 0) {

            // Found a factor pair.
            factorSum += lowFactor;
            int highFactor = num / lowFactor;

            // Only add square root once for square numbers.
            if (highFactor != lowFactor) {
                factorSum += highFactor;     
            }
        }            
    }

    return factorSum == num;

} // end isPerfect()