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

Finding the perfect number from 1 to n

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numbertheperfectfindingfrom

Problem

I have a program to find the perfect number from 1 to n:

How can this be reduced without changing it?

class PerfectNumber
{
    public static StringBuffer fact(int a)
    { 
        StringBuffer sb=new StringBuffer(" ");
        for(int i=1;i<a;i++)
        if(a%i==0)
        {
            sb.append(i+",");
        }
        return sb; 
    }
    public static String isPrime(int a)
    {
        int sum=0;
        for(int i=1;i<a;i++)
        {
            if(a%i==0)
        {
            sum+=i;
        }   

        }
        if (sum==a)
        {
            return "true";
        }
        else
            return "false";
    }
    public static int perfecrNo(int a)
    {
        int sum=0;
        for(int i=1;i<a;i++)
        {
            if(a%i==0)
        {
            sum+=i;
        }   

        }
        return sum;
    }
    public static void main(String[] args)
    {

        System.out.println("--------------------------------------------------------------------------");
        System.out.printf("%s%40s%15s%15s%n","NO","fact","sum","isPrime");
        System.out.println("--------------------------------------------------------------------------");
        for(int i=1;i<100;i++)
        {
            System.out.printf("%n %d %40s\b %15d %15s",i,fact(i),perfecrNo(i),isPrime(i));
        }

    }
}

Solution

Indentation

To pick one example:

for(int i=1;i<a;i++)
        {
            if(a%i==0)
        {
            sum+=i;
        }


What's going on here? My best guess would be that you're mixing tabs and spaces with a tabstop of 8. Be consistent: tabs or spaces. Changing the tabstop shouldn't break the formatting.

Names

public static StringBuffer fact(int a)
    public static String isPrime(int a)
    public static int perfecrNo(int a)


What do these three methods do?

  • The first one produces a string containing the factors. Before reading the implementation I had guessed that it calculated the factorial. Name could be more descriptive: e.g. formatFactors.



  • The second one tests whether the number is a perfect number or not. The name is simply wrong: it claims that the method does something different to what it does. It should be e.g. isPerfectNumber.



  • The third one calculates the sum of the factors. The name certainly doesn't communicate that to me. It should be e.g. sumOfFactors (or aliquotSum if you don't mind the technical term).



Factoring out common code

All three of those methods have a loop which does the same thing: find the factors of a. That should be pulled out into a single method, which should have a suitable return type (e.g. Collection, although there's a good case for Set, or maybe some integer stream type, but I'm not up to date on Java 8).

Then two of the methods sum the factors: that's already pulled out into a method, so the other method shouldn't duplicate it. Instead you should have a very simple method

public static boolean isPerfectNumber(int a)
{
    return a == sumOfFactors(a);
}


Rethink method signatures

Why does fact return a StringBuffer? I can't think of many situations in which it makes sense to return a StringBuffer (or a StringBuilder). Either you're appending to a StringBuffer/StringBuilder, in which case it should almost always be an argument and doesn't need to be returned, or you aren't, in which case it makes more sense to return a String.

Why does isPrime (isPerfectNumber) return a string "true" or "false"? The boolean type exists for precisely the type of things which are true or false.

Code Snippets

for(int i=1;i<a;i++)
        {
            if(a%i==0)
        {
            sum+=i;
        }
public static StringBuffer fact(int a)
    public static String isPrime(int a)
    public static int perfecrNo(int a)
public static boolean isPerfectNumber(int a)
{
    return a == sumOfFactors(a);
}

Context

StackExchange Code Review Q#163375, answer score: 10

Revisions (0)

No revisions yet.