5
\$\begingroup\$

This is my first Java program to print out the Armstrong numbers.

An Armstrong number is a number that is the sum of its own digits each raised to the power of the number of digits.

I want to get better, so be harsh and straightforward.

public class Armstrong {

    public static void main (String arg[]){

        int Max=2000000000;
        for (int i=0; i<Max; i++){
            if (isarmstrong(i)){
                System.out.println(i);
            }

        }
    }

    private static boolean isarmstrong(int x) {
        int n=x;
        if (n == getdig(n)){
            return true;
        }
        else return false;

    }


    private static int getdig(int sp){

        int d=sp;
        int r=0, dg=0;
        int length = String.valueOf(sp).length();
     if (length>1){
        for (int count=length; count>=0; count--){

            dg = (int) (d/(Math.pow(10, count)));
            d= (int) (d % (Math.pow(10, count)));
            dg =(int) Math.pow(dg, length);
            r = r + dg;
        }

        }
     else {
         r=sp;
     }
     return r;

        }
}
\$\endgroup\$
4
\$\begingroup\$

Code formatting

There a some inconsistencies in your code formatting. Compare these functions:

public static void main (String arg[]){

    int Max=2000000000;
    for (int i=0; i<Max; i++){
        if (isarmstrong(i)){
            System.out.println(i);
        }

    }
}

private static boolean isarmstrong(int x) {
    int n=x;
    if (n == getdig(n)){
        return true;
    }
    else return false;

}
  1. Sometimes you use no space before a {, like this: ){. Sometimes you do have a space: ) {.
  2. Sometimes you have a blank line before you declare your variables. Sometimes you don't.
  3. Sometimes you have a blank line before the end of your function. Sometimes you don't.

It would be helpful to keep consistent in the way you format your code. It makes it easier for a reader to parse. Also, you have some weird indentation going on in your last function.

isarmstrong() function

First of all, the function as written is way too complicated for what it does. It could be rewritten as return x == getdig(x);. Second of all, since it is really so simple, I would just eliminate the function and rewrite the code in main() to be:

    if (i == armstrongValue(i)) {
        System.out.println(i);
    }

getDig() function

The first thing I noticed was that you have a special case for length <= 1. That special case doesn't need to exist. Your code works fine for length 1 numbers and checking for the special case is just going to slow things down.

The second thing I noticed is that you start the loop at count = length, but the first iteration of that loop will never do anything because dg will always be 0 on the first iteration. You should start count at length-1 instead. So the function could be rewritten as:

private static int armstrongValue(int num) {
    int ret = 0;
    int length = String.valueOf(num).length();
    for (int count = length-1; count >= 0; count--) {
        int digit = (int) (num / Math.pow(10, count));
        ret += Math.pow(digit, length);
        num %= Math.pow(10, count);
    }
    return ret;
}

Math.pow()

Your program runs pretty slowly. There are several optimizations you could make. The first one is that you are using Math.pow() a lot. Math.pow() takes double arguments and returns a double. It isn't optimized for integer powers. If you used a function that handled integer powers efficiently, it would work a lot better:

private static int intPow(int base, int exp) {
    int ret = 1;
    while (exp > 0) {
        if ((exp & 1) != 0) {
            ret *= base;
        }
        exp >>= 1;
        base *= base;
    }
    return ret;
}

When I replaced each call to Math.pow() with intPow(), your program ran approximately 14x faster than before.

Duplicating work

Your getDig() function converts the number to a string to find the length of the number, which is fine. But then it does a bunch of math to compute each digit. Since you already converted the number to a string, you should already have all the digits. They are simply the characters of the string. If you just read out the characters of the string, you would save a lot of work. Here is the new function:

private static int armstrongValue(int num) {
    String str = String.valueOf(num);
    int    len = str.length();
    int    ret = 0;

    for (int i=0; i<len; i++) {
        int digit = str.charAt(i) - '0';
        ret += intPow(digit, len);
    }
    return ret;
}

This version runs 1.7x faster than the previous version, or 23x faster than the original version.

Putting it all together

Here is your program with all of the changes mentioned above:

public class Armstrong {

    private static final int DEFAULT_ITERATIONS = 2000000000;

    public static void main(String arg[]) {
        int Max = DEFAULT_ITERATIONS;

        // Get iterations from command line, if specified.
        if (arg.length > 0) {
            Max = Integer.parseInt(arg[0]);
        }

        for (int i=0; i<Max; i++) {
            if (i == armstrongValue(i)) {
                System.out.println(i);
            }
        }
    }

    private static int armstrongValue(int num) {
        String str = String.valueOf(num);
        int    len = str.length();
        int    ret = 0;

        for (int i=0; i<len; i++) {
            int digit = str.charAt(i) - '0';
            ret += intPow(digit, len);
        }
        return ret;
    }

    private static int intPow(int base, int exp) {
        int ret = 1;
        while (exp > 0) {
            if ((exp & 1) != 0) {
                ret *= base;
            }
            exp >>= 1;
            base *= base;
        }
        return ret;
    }
}

Some other miscellaneous things I changed in your program:

  1. Moved the constant 2000000000 to a private static final int.
  2. Added a way to specify the max number from the command line. Even the fastest version of the program still took 4 minutes to run at 2 billion iterations. So I tested with small iteration counts.

Footnote: I was able to get the program to run even faster by caching all of the relevant intPow() results in an int[10][11] array and looking them up instead of calling intPow(). The cached version ran 36x faster than the original, or about 1.6x faster than the uncached version. Essentially I replaced ret += intPow(digit, len); with ret += powCache[digit][len];.

\$\endgroup\$
5
\$\begingroup\$

Your indenting is weird. In getdig, you deindent, then reindent.

Variables should always be camelCase or, if they're constants, UPPER_SNAKE_CASE -- int Max should be int max or final int MAX, depending on if you want to make it clear that it's a constant. I also think it should be a class variable, not a local variable in a method.

Put a space on either side of your binary (two-argument) operators -- so int n=x becomes int n = x, i<max becomes i < max, etc. Note that things like ++ and ! are not binary, since they only take one argument.

Method names are camelCase as well, where the humps are on word boundaries. isarmstrong should be isArmstrong.

Use meaningful names! sp, getdig, n, sp -- none of these mean anything. It's fine to have long names -- don't use any abbreviations but the most obvious ones. Id sit, getIP would be fine; you don't need to write getInternetProtocolAddress.

if (/* condition */) {
    return true;
}
return false;

is a pretty common way to add lots of characters with no meaning. Just write

return /* condition */;

replacing /* condition */ with the actual code. In this case, that's n == getDig(n).

You have a bunch of lines with nothing but whitespace; the convention I've seen is to separate methods and logically distinct blocks of code. You have a bunch of whitespace just before the method ends, or just after it begins.

In isArmstrong, you assign n to be equal to x, then never use x again. Why not ditch n and just use the original variable?

In getdig's for, a couple of things.

Firstly, a = a + b can be shortened to a += b and it's understood what that means.

Secondly, since none of the other code relies on the changing of d -- in fact, you moved one line to keep it from interfering -- why not visually separate it by moving it to the bottom and plopping a blank line between?

As for your actual math, though, it looks good. It's rather hard to tell, since your names mean absolutely nothing, but no matter.

With all of these applied (except the better names -- you should do that yourself, since you know what they are) here's how your code looks:

public class Armstrong {
    public static void main (String arg[]){
        int max = 2000000000;
        for (int i = 0; i < max; i++){
            if (isArmstrong(i)){
                System.out.println(i);
            }
        }
    }

    private static boolean isArmstrong(int n) {
        return n == getDig(n);
    }

    private static int getDig(int sp){
        int d = sp;
        int r = 0, dg = 0;
        int length = String.valueOf(sp).length();
        if (length > 1){
            for (int count = length; count >= 0; count--){
                dg = (int) (d / Math.pow(10, count));
                dg = (int) Math.pow(dg, length);
                r += dg;

                d = (int) (d % Math.pow(10, count));
            }

        }
        else {
            r = sp;
        }
        return r;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.