2
\$\begingroup\$

This is my code to generate all possible Armstrong numbers between the two given numbers. The logic uses string instead of integer to separate the digits to optimize the code.

import java.util.Scanner;

public class ArmstrongNumberGenerator {
    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        int firstNumber;
        int lastNumber;
        int sum = 0;
        try {
            System.out.println("\nYou will have to enter initial and final number between which all the armstrong numbers you want to generate\n");
            System.out.println("\nEnter the initial number\n");
            firstNumber = scanner.nextInt();
            System.out.println("\nEnter the final number\n");
            lastNumber = scanner.nextInt();
            if (firstNumber == lastNumber) {
                System.out.println("both initian and final numbers are same , no range to generate armstrong numbers");
            } else {
                if (firstNumber > lastNumber) {
                    System.out.println("initial number is greater than final number so i will alter them and make a range from " + lastNumber + " to " + firstNumber);
                    int temp = firstNumber;
                    firstNumber = lastNumber;
                    lastNumber = temp;
                }
                do {
                    String s = Integer.toString(firstNumber);
                    char[] c = s.toCharArray();
                    for (int i = 0; i < s.length(); i++) {
                        sum = ( int ) (sum + Math.pow((c[i] - 48), c.length));
                    }

                    if (sum == firstNumber) {
                        System.out.println("Number " + firstNumber + " is Armstrong");
                    }
                    ++firstNumber;

                    sum = 0;
                } while (firstNumber < lastNumber);

            }
        } catch (Exception e) {
            System.out.println("invalid data");
        }

    }
}
\$\endgroup\$
2
\$\begingroup\$

Hello and thanks for sharing your code with us.

Readability/Maintainability

  1. Variables should only be declared when they are actually used (Unless you are having to work with different scopes). It can become difficult to keep track of what is what when everything is just declared at the top of our scope.
  2. We should try to stay away from deeply nested structures. Very rarely will you be forced to go even two scopes deep.
  3. When catching exceptions, it is generally best to catch specific exceptions. Although, in this scenario we can afford to be a bit more liberal.Here you can find more details on exception handling

Lets look at some refactored code to see what these changes might look like. Also, take note of various comments left throughout the code base.

public static void main(String[] args) {
    System.out.println("\nYou will have to enter initial and final number between which all the armstrong numbers you want to generate\n");
    int start;
    int end;
    try {
        start = promptForNextNumber("\nEnter the initial number\n");
        end = promptForNextNumber("\nEnter the final number\n");
    } catch (InputMismatchException ex) {
        System.out.println("Input was not a valid integer.");
        return;
    }

    if (start == end) {
        System.out.println("both initial and final numbers are same, no range to generate armstrong numbers");
        return;
    }

    if (start > end) {
        final String message = String.format(
                    "initial number is greater than final number so i will alter them and make a range from %s to %s", end, start);
        System.out.println(message);
        // This is a strange and not recommended way of handling this situation.
            // But if this functionality is apart of the requirement, by all means.
        int temp = start;
        start = end;
        end = temp;
    }

    ...
}

Take notice of the promptForNextNumber(message) method being called. In command line applications it is very common to prompt a user than collect input. We can capitalize on this pattern so that we do not repeat our selves. The method looks like this:

private static int promptForNextNumber(final String message) {
    System.out.println(message);
    return scanner.nextInt();
}
  1. Lets try to keep our functionality separate in their own methods, this way things can be easily reused if needed and gets rid of the giant mother block of code. Code is easier to understand when it is broken up into tinier chunks. We'll see examples of this in just a bit.

Alternate Solution

As @TorbenPutkonen has already pointed out, your algorithm for determining if a number is armstrong or not is a bit harder to follow than it should be. Although imperative programming gets the job done it can be on the more verbose side, even when done correctly. I would like to propose a functional solution:

private static boolean isArmstrong(final String number) {
    final int length = number.length();
    final int sum = number.chars()
            .map(Character::getNumericValue)
            .map(digit -> (int) Math.pow(digit, length))
            .sum();

    return sum == Integer.parseInt(number);
}

and an overload for easy type converting

// Method overload for easy conversion from int to string.
private static boolean isArmstrong(final int number) {
    return isArmstrong(String.valueOf(number));
}

We now have methods for determining if a given number is an Armstrong number. Lets use them by first generating a range of numbers and then filtering that range with our new methods. After the filtering process, simply print the results.

public static void main(String[] args) {
    ...


    IntStream.range(start, end)
            .filter(ArmstrongMainRevisioned::isArmstrong)
            .forEach(number -> System.out.println("Number " + number + " is Armstrong"));
}

Bringing It All Together

This is just one of many possible ways this application could be written using these various mentioned techniques.

import java.util.InputMismatchException;
import java.util.Scanner;
import java.util.stream.IntStream;

public class ArmstrongNumberGenerator {
    private static final Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {
        System.out.println("\nYou will have to enter initial and final number between which all the armstrong numbers you want to generate\n");
        int start;
        int end;
        try {
            start = promptForNextNumber("\nEnter the initial number\n");
            end = promptForNextNumber("\nEnter the final number\n");
        } catch (InputMismatchException ex) {
            // Lets display a slightly more descriptive message describing why the given data was invalid.
            System.out.println("Input was not a valid integer.");
            return;
        }

        if (start == end) {
            System.out.println("both initial and final numbers are same, no range to generate armstrong numbers");
            return;
        }

        if (start > end) {
            // String.format can be used to improve string readability when concatenating a lot of different strings.
            final String message = String.format(
                    "initial number is greater than final number so i will alter them and make a range from %s to %s", end, start);
            System.out.println(message);
            // This is a strange and not recommended way of handling this situation.
            // But if this functionality is apart of the requirement, by all means.
            int temp = start;
            start = end;
            end = temp;
        }

        IntStream.range(start, end)
                .filter(ArmstrongMainRevisioned::isArmstrong)
                .forEach(number -> System.out.println("Number " + number + " is Armstrong"));
    }

    private static boolean isArmstrong(final String number) {
        final int length = number.length();
        final int sum = number.chars()
                .map(Character::getNumericValue)
                .map(digit -> (int) Math.pow(digit, length))
                .sum();

        return sum == Integer.parseInt(number);
    }

    // Method overload for easy conversion from int to string.
    private static boolean isArmstrong(final int number) {
        return isArmstrong(String.valueOf(number));
    }

    // Helper method to display prompt while acquiring user input
    private static int promptForNextNumber(final String message) {
        System.out.println(message);
        return scanner.nextInt();
    }
}

\$\endgroup\$
  • \$\begingroup\$ thanks for your review . I will try to code better . \$\endgroup\$ – Akshay soni May 3 at 3:33
3
\$\begingroup\$

The first number is included in the range but the last number is not. That inconsistency is odd. You should document the limitations you set to the input. If your limitations make documentation hard, it's a sign of bad programming.

Knowing what I wrote above, right now you to check for both equality and greater than between firstNumber and lastNumber. Just check if (firstNumber > lasNumber) instead and tell the user that "firstNumber must be smaller than lastNumber."

FirstNumber and lastNumber are not descriptive variable names. LowerLimit and upperLimit would be better.

You're not prepared for negative input.

Using firstNumber as both the lower limit and loop counter makes the variable name to be incorrect in both uses. It's really never the lowerLimit nor the number being checked. Add a separate variable and use a loop for (int candidate = lowerLimit; candidate <= upperLimit; candidate++) { ...

You should separate the algorithm from main method that reads the input to a static utility method that operates on integers. Reading code that is nested four deep is difficult.

\$\endgroup\$
  • 2
    \$\begingroup\$ You don't seem to understand the purpose of Code Review. We're working together to improve the skills of programmers worldwide by taking working code and making it better. Please see codereview.stackexchange.com/tour \$\endgroup\$ – TorbenPutkonen May 2 at 9:55
  • \$\begingroup\$ I agree with most of what is written in your answer but I can't vote it up because of the tone. Is it possible for your reviews to address only the code and not the author of the code by the heavy use of "you"? \$\endgroup\$ – pacmaninbw May 2 at 10:35
  • 1
    \$\begingroup\$ @pacmaninbw It's likely the product of my natural language, which is not english. I'll try to look into it in the future. \$\endgroup\$ – TorbenPutkonen May 2 at 10:41
0
\$\begingroup\$

Thanks for your reviews everyone. I updated my code using the answers and updates in my new code are as below.

I now use my void main () only to create an object that can run this code , like this.

public static void main(String[] args) {
        new ArmstrongNumberGenerator().mainMethod();
    }

I distributed the tasks of getting input and doing calculations by different methods and also included the lastnumber in the range.

private void mainMethod() {
        try {
            System.out.println("\nYou will haveto enter initial and final number between which all the armstrong numbers you want to generate\n");
            System.out.println("\nEnter the initial number\n");
            firstNumber = scanner.nextInt();
            System.out.println("\nEnter the final number\n");
            lastNumber = scanner.nextInt();
            armstrongGenerator(firstNumber, lastNumber);
        } catch (InputMismatchException e) {
            System.out.println(e + "\t: Only integers allowed as input");
        }
    }
private void armstrongGenerator(int firstNumber, int lastNumber) {
        if (firstNumber == lastNumber) {
            System.out.println("both initian and final numbers are same , no range to generate armstrong numbers");
        } else {
            if (firstNumber > lastNumber) {
                System.out.println("initial number is greater than final number so i will alter them and make a range from " + lastNumber + " to " + firstNumber);
                int temp = firstNumber;
                firstNumber = lastNumber;
                lastNumber = temp;
            }
            do {
                String s = Integer.toString(firstNumber);
                char[] c = s.toCharArray();
                for (int i = 0; i < s.length(); i++) {
                    sum = ( int ) (sum + Math.pow((c[i] - 48), c.length));
                }
if (sum == firstNumber) {
                    System.out.println("Number " + firstNumber + " is Armstrong");
                }
                ++firstNumber;
                sum = 0;
            } while (firstNumber <= lastNumber);
        }
    }
\$\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.