7
\$\begingroup\$

I am going through the Java CodingBat exercises. Here is the one I have just completed:

Given a string, return the sum of the digits 0-9 that appear in the string, ignoring all other characters. Return 0 if there are no digits in the string.

Here is my code:

public int sumDigits(String str) {

    StringBuilder strAppend = new StringBuilder();

    for (int i = 0; i < str.length(); i++) {
        if (Character.isDigit(str.charAt(i))) {
            strAppend.append(str.charAt(i));
        }
    }

    int total = 0;
    String strDigits = strAppend.toString();

    for (int i = 0; i < strDigits.length(); i++) {
        total += Integer.parseInt(strDigits.substring(i, i+1));
    }

    return total;

}

My questions are:

  1. Should I be using one for loop instead of two?
  2. Is it correct to be converting to StringBuilder to avoid concatenating a string?
  3. Is it right to convert from string to int towards the end of the method, or convert as the digits are found?
\$\endgroup\$
11
\$\begingroup\$

Your question is surprising in the sense that you use the somewhat obscure isDigit method (not many people are aware of that one), yet you do not use the similar Character.getNumericValue(...) method.

Using that method, you can remove the second loop (as you suspected). It also answers the second question - there is no need for a StringBuilder, or the int conversion:

int total = 0;
for (char c : str.toCharArray()) {
    if (Character.isDigit(c)) {
        total += Character.getNumericValue(c);
    }
}
return total;
\$\endgroup\$
  • \$\begingroup\$ Ah - I was just going off the note as part of the question which reads: (Note: Character.isDigit(char) tests if a char is one of the chars '0', '1', .. '9'. Integer.parseInt(string) converts a string to an int.) but getNumericValue() is way more useful - I struggle to see why it wasn't suggested by the CodingBat question \$\endgroup\$ – alanbuchanan Apr 13 '15 at 12:07
3
\$\begingroup\$

The semantics of sumDigits is to sum all digits in input string and return this sum. So storing to to StringBuilder is an implementation internal detail. Actually, it doesn't offer anything to the clarity of code and from a performance point of view it adds the unnecessary overhead of the StringBuilder creation.

Also as pointed out the method offered from Java to extract the numeric value of a digit is through Character.getNumericValue, which provides readability and cross platform support.

Finally, there is no reason restricting to strings, since you can use any CharSequence instance as input:

public int sumDigits(CharSequence s) {
    int total = 0;
    int length = s.length();
    for (int i = 0; i < length; i++) {
        char c = s.charAt(i);
        if (Character.isDigit(c))
            total += Character.getNumericValue(c);
    }
    return total;
}
\$\endgroup\$
3
\$\begingroup\$

Since nobody did an answer like that yet, I'd like to provide an alternative solution using Java-8 streams.

The basic parts are already present in your existing code and the rest is leveraging streams:

public int sumDigits(String str) {
    return str.chars().mapToObj(i -> (char) i).filter(Character::isDigit)
       .mapToInt(Character::getNumericValue).sum();
}

The code should be rather self-explanatory, I think it's clear ;)

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

One loop, once you encounter a digit, you should sum it up already. :) This answers your third question.

Adding the digit character to a StringBuilder, then having to parse each digit afterwards, is redundant.

\$\endgroup\$

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