4
\$\begingroup\$

There is a custom summary function needed as to receive an integer input (e.g. 32456) and returns 3+2+4+5+6. Can you think of any other (or better) solution besides this?

public class CustomSum {
    public int sum(int input) {
        int sum = 0;

        while ( input >= 1 ) {
            sum += input % 10;
            input = input / 10;
        }

        return sum;
    }

    public static void main(String[] args) {
        int input = Integer.parseInt(args[0]); //assuming args[0] is integer

        CustomSum c = new CustomSum();
        int sum = c.sum(input);

        System.out.println("sum: " + sum);
    }
}

In every iteration I intended to extract the right digit by input % 10, and omit the extracted right digit for next iteration by input / 10. Now consider when we are in last iteration and have a 1 digit number. The input / 10 would be greater than 0 and less than 1 (if actually it was float or double not int). So I didn't actually changed that specific condition, although with int it will always be 0.

\$\endgroup\$
  • \$\begingroup\$ So you want to enter digits 32456 and you want the sum? Why would you do it like this? If I entered 355 what's to differentiate 35 + 5 or 3 + 5 + 5 or 3 + 55? \$\endgroup\$ – Legato Mar 14 '15 at 17:57
  • \$\begingroup\$ The requirement is this, to get an in integer and sum all the digit in it. So 355 would be 3+5+5 not 35+5 or 3+55 \$\endgroup\$ – Khosrow Mar 14 '15 at 18:01
  • \$\begingroup\$ I think this is a good implementation for the sum of digits. The method sum itself can be made static since this is a function. \$\endgroup\$ – T.Gounelle Mar 14 '15 at 18:41
  • 2
    \$\begingroup\$ Given your edit, you should still follow the advice of @rolfl and Brythan. You shouldn't make programming decisions based on what would happen if you were using a float or double when you're in the midst of using ints... You're using an int. An int can't be any number between 0 and 1. So use > 0 so that no one asks "Why the hell does this say >= 1?" \$\endgroup\$ – nhgrif Mar 14 '15 at 19:18
6
\$\begingroup\$

Your sum(...) function is fine, given the specification, but there is jut an issue of input validation... what about negative values?

Your current loop condition is input >= 1. This would be better written as input > 0. When you see the >= 1 it makes you wonder if there's a condition that's weird. In this case, there's not.

Further, your code excludes all negative values. What about the sum of -123?

That makes me think that the code should all be looped on the condition input != 0, but that would sum -123 as -6, and not 6.

I was initially tempted to suggest that you have the code:

    input = Math.abs(input);

    int sum = 0;

    while ( input != 0 ) {
        sum += input % 10;
        input = input / 10;
    }

    return sum;

Unfortunately, that has a flaw with Integer.MIN_VALUE (the abs of Integer.MIN_VALUE is.... Integer.MIN_VALUE - a negative number). The net result, is that I considered doing an abs on each digit value inside the loop, but then realized, that all the digits will be negative, so the 'sum' will just be a large negative value, and we can take the absolute of the sum safely, because the sum of the digits will never be large enough to be a problem...

Thus, the solution I would recommend is:

public static int sumSumAbs(int input) {

    int sum = 0;

    while ( input != 0 ) {
        sum += input % 10;
        input = input / 10;
    }

    return Math.abs(sum);
}

That code (adjusted to fit in to my Eclipse IDE with a static method call, and different name), will work for any input value, of any sign, and ignore the sign in the result.

\$\endgroup\$
  • \$\begingroup\$ +1 on the negative approach, although the negative value isn't the concern of original problem. see my edit on question in regard of input >= 1 \$\endgroup\$ – Khosrow Mar 14 '15 at 19:04
4
\$\begingroup\$

Alternative Method

If you just want an alternative method:

    public static int sumDigits(String digits) {
        int sum = 0;

        for ( Character digit : digits.toCharArray() ) {
            if ( ! Character.isDigit(digit) ) {
                throw new IllegalArgumentException("Character not a digit in [" + digits + "].");
            }

            sum += digit;
        }

        // subtract out a '0' for each digit
        // could have done this as we added, but this way is fewer operations
        return sum - '0' * digits.length();
    }

This avoids the use of parseInt. It looks like it works at least to the range of what an integer can hold. I.e. parseInt stops working before it does on my system (a value of 2,147,483,648 is too large for parseInt).

Note that this throws an exception on negative numbers and possibly other formats that parseInt would support. It will also return incorrect values when the sum of the character values exceeds the capacity of an int.

Naming

public class CustomSum {

I would name this DigitsSum instead. That is more descriptive, as it explains what is custom about it.

    public int sum(int input) {

As Hosch250 noted there is no reason this shouldn't be a static method. You make no use of state in the object.

    public static int sumDigits(int number) {

If you call it sumDigits instead it makes it clearer that it is not generating a normal sum. Also, changing input to number makes it clearer what the argument holds (which may not be an input value).

Say what you mean

        while ( number >= 1 ) {

Is 1 important here? Or are you just saying that it is not 0 and not negative?

        while ( number > 0 ) {

While this is functionally equivalent to the previous statement, it does a better job of separating the terminal condition (0) from other values. I.e. in normal operation, this will stop looping when number is equal to 0. So say it that way.

Alternative method 2

    public static int sumDigits(String digits) {
        int sum = 0;

        for ( Character digit : digits.toCharArray() ) {
            if ( ! Character.isDigit(digit) ) {
                throw new IllegalArgumentException("Character not a digit in " + digits);
            }

            sum += Character.getNumericValue(digit);
        }

        return sum;
    }

This just uses a different method of converting from character values to digit values. This is consistently slower than your version in my testing though.

Testing

    public static void main(String[] args) {
        String input = "0047483647";
        int sum;
        long start, stop;

        start = System.nanoTime();
        int number = Integer.parseInt(input);

        sum = sumDigits(number);
        stop = System.nanoTime();

        System.out.println("sum: " + sum);
        System.out.println("Execution time: " + ((stop - start) / 1e+6) + " ms");

        start = System.nanoTime();
        sum = sumDigits(input);
        stop = System.nanoTime();

        System.out.println("sum: " + sum);
        System.out.println("Execution time: " + ((stop - start) / 1e+6) + " ms");
    }

I converted from args[0] to a constant because it made it easier for me to test. If your setup is different, the reverse may be true.

I found that if I ran this once, my version (the first one) was faster. If I ran it a second or third time (in the same execution), your version was as fast if not faster. Both version returned the same output on the values that I tested.

\$\endgroup\$
  • \$\begingroup\$ I originally thought of something like putting input.substring(...) in a loop, but digits.toCharArray() is way more better. see my edit on question in regard of input >= 1 \$\endgroup\$ – Khosrow Mar 14 '15 at 19:06
4
\$\begingroup\$

I want to review the structure of your code here. rolfl's answer does an excellent review of the sum method, and I'll point you in that direction for the internals of the method, but I want to talk about some bigger picture things here.

We should think of instances of objects as being things. Specifically, things that we do something to (by calling methods on). That means that when we instantiate an object, we have a thing. And any method we call on that thing will either give us output based on the state of that thing, or it will change the state of that thing.

A method which takes an input and gives us an output based purely on the input doesn't meet this criteria. These sorts of methods should be static class methods--not instance methods.

And in the big picture, the class that contains main should do nothing more than launch our application.

So what we actually want is a utility class, of which we won't instantiate any objects, and within it, we'll implement static methods:

public class CustomMath {
    public static int sumOfDigits(int input) {
        // your implementation here
    }

    // might include other methods in here... productOfDigits? etc.
}

And then in the file with the main method, it might look something like this:

public class CustomMathTests {
    public static void main(String[] args) {
        int input = Integer.parseInt(args[0]); //assuming args[0] is integer
        int sum = CustomMath.sumOfDigits(input);
        System.out.println("sum: " + sum);
    }
}

Someone who knows Java better than I could comment as to whether there's a way to prevent a class from being instantiated.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the your description. As I mentioned codereview.stackexchange.com/questions/84083/…, my intention was only the implementation of sum method, and just didn't expect that you guys even consider everything around it. so I just put it in static void main() to represent the Java being used. \$\endgroup\$ – Khosrow Mar 14 '15 at 19:28
  • \$\begingroup\$ and by the way we can use public final class CustomMath and private constructor to prevent it to be instantiated. \$\endgroup\$ – Khosrow Mar 14 '15 at 19:31
2
\$\begingroup\$

Why do you create a new class for each sum? This section:

CustomSum c = new CustomSum();
int sum = c.sum(input);

Should probably just be:

int sum = sum(input);

As for the sum() method, I think this is a good implementation, but other people might have more comments.

\$\endgroup\$
  • \$\begingroup\$ Thanks but my question is regarding the implementation of sum method rather than how it is called. \$\endgroup\$ – Khosrow Mar 14 '15 at 17:34
  • \$\begingroup\$ @Khosrow Hosch is right, you don't need a class just for this, though saving the method in a utility class would be acceptable. \$\endgroup\$ – Legato Mar 14 '15 at 18:32

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.