6
\$\begingroup\$

I wrote this program, to determine if any given number is happy, and would like to know of any improvements that can be made to make it better. I have tested it, but whether or not it works perfectly I have no idea, there might be some numbers that get through, although I doubt it.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt(), value;

        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            value = 0;
            for (char c : String.valueOf(number).toCharArray())
                value += Math.pow(Integer.parseInt(String.valueOf(c)), 2);
            number = value;
        }

        System.out.println(number == 1 ? "Happy" : "Not Happy");
    }
}
\$\endgroup\$

7 Answers 7

11
\$\begingroup\$

Apart from what @RubberDuck said, you can also do a bit less work by not converting from/to String, but iterating over the digits one by one directly. I've also moved the value to where it is initialised, that way its scope is better visible.

After making those changes I arrive at the following snippet. isHappy or (isHappyNumber) can be reused and the main function only does interface stuff.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static boolean isHappy(int number)
    {
        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            int value = 0;
            while (number > 0)
            {
                value += Math.pow(number % 10, 2);
                number /= 10;
            }
            number = value;
        }

        return number == 1;
    }

    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt();
        System.out.println(isHappy(number) ? "Happy" : "Not Happy");
    }
}
\$\endgroup\$
0
10
\$\begingroup\$

In addition to the point made by RubberDuck there's some stuff you should definitely change in addition to that:

    int number = new Scanner(System.in).nextInt(), value;

Don't do this. Every variable declaration has the right to stand on its own. Glossing over this, one could miss the declaration of value. Additionally if your input is not an int you'll get an InputMismatchException if the user didn't enter an integer.

There is also a much easier way to get every digit of a number, instead of the bunch of unintuitive String transformations you are doing.

int lowestSignificantDigit = number % 10;
number = number / 10;

You just repeat the code above until there is no number left ;)


Additionally I think that the ternary inside your System.out.println is rather unclear. As soon as you extract the determination of Happy / Not Happy you should simply use the return value of the method you extracted.

\$\endgroup\$
6
\$\begingroup\$

It's a bit of a moot point, but this is not a great way to convert a character digit to integer:

int digit = Integer.parseInt(String.valueOf(c));

This is better:

int digit = c - '0';

Of course this will only work if the value of c is within the range '0' to '9'.

As others have already said, create an boolean isHappy(int) method to separate its logic from everything else. It will be a nice method with a single responsibility.

Once you have such method, it's good to add unit tests to verify it works correctly:

@Test
public void test_19_Is_Happy() {
    assertTrue(isHappy(19));
}

@Test
public void test_22_Is_NotHappy() {
    assertFalse(isHappy(22));
}

To be more thorough, you can take the list of known happy numbers from this wikipedia page and turn that into a test:

@Test
public void testKnownHappy() {
    Set<Integer> happy = new HashSet<>();
    happy.addAll(Arrays.asList(1, 7, 10, 13, 19, 23, 28, 31, 32, 44, 49, 68, 70, 79, 82, 86, 91, 94, 97, 100, 103, 109, 129, 130, 133, 139, 167, 176, 188, 190, 192, 193, 203, 208, 219, 226, 230, 236, 239, 262, 263, 280, 291, 293, 301, 302, 310, 313, 319, 320, 326, 329, 331, 338, 356, 362, 365, 367, 368, 376, 379, 383, 386, 391, 392, 397, 404, 409, 440, 446, 464, 469, 478, 487, 490, 496, 536, 556, 563, 565, 566, 608, 617, 622, 623, 632, 635, 637, 638, 644, 649, 653, 655, 656, 665, 671, 673, 680, 683, 694, 700, 709, 716, 736, 739, 748, 761, 763, 784, 790, 793, 802, 806, 818, 820, 833, 836, 847, 860, 863, 874, 881, 888, 899, 901, 904, 907, 910, 912, 913, 921, 923, 931, 932, 937, 940, 946, 964, 970, 973, 989, 998, 1000));
    for (int i = 0; i < 1000; ++i) {
        assertEquals(happy.contains(i), isHappy(i));
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ ++ for mentioning proper tests. \$\endgroup\$
    – RubberDuck
    Nov 30, 2014 at 19:14
  • \$\begingroup\$ Character.digit(c, 10) is another alternative to Integer.parseInt(String.valueOf(c)). \$\endgroup\$ Dec 1, 2014 at 7:32
5
\$\begingroup\$

The "Happy Number" code is tightly bound to your main routine and user interface. As it is, you couldn't take the code and plug it into another program. It would be best to break this code out into its own class and call it from Main.

\$\endgroup\$
3
\$\begingroup\$

Since all unhappy numbers enter the sequence beginning with 4 there is no need to store the list of numbers. Just test for 4 or 1.

\$\endgroup\$
0
\$\begingroup\$

Math.pow() is an inefficient (and possibly inaccurate) way to square small numbers. It's much better to simply multiply:

        for (char c: String.valueOf(number).toCharArray()) {
            int digit = Character.digit(c, 10);
            value += digit * digit;
        }

Of course, this improvement can also be combined with better ways of extracting digits:

        for (;  number > 0;  number /= 10) {
            int digit = number % 10;
            value += digit * digit;
        }

If you're trying to squeeze every last cycle of performance, a ten-element lookup table may be even faster. But profile properly if speed matters so much.

\$\endgroup\$
-2
\$\begingroup\$

public class HappyNumber {

public static void sumHappy(int number){
    int sum =0;
    int r =0;
    while(number>0){
        r = number%10;
        sum = sum + (r*r);
        number = number/10;         
    }
    if (sum!= 1 && sum > 9){
        sumHappy(sum);
    }
    else if(sum == 1){
        System.out.println("Happy number");
    }
    else{
        System.out.println("Not a Happy number");
    }
}
public static void main(String[] args) {
    int num = 91;
    sumHappy(num);
}

}

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Please explain why your solution is better than the one in the question. A code dump like this is not particularly useful. \$\endgroup\$
    – Null
    Oct 15, 2018 at 22:13
  • 2
    \$\begingroup\$ Welcome to Code Review! Just as @Null stated: You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. Please read Why are alternative solutions not welcome? \$\endgroup\$ Oct 15, 2018 at 22:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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