Problem
I am going through the CodingBat exercises for Java. Here is the task I have just completed:
Given a
string
, return the sum of the numbers appearing in thestring
, ignoring all other characters. A number is a series of 1 or more digit chars in a row.
And here is my code:
public int sumNumbers(String str){
int total = 0;
for (int i = 0; i < str.length(); i++) {
if (Character.isDigit(str.charAt(i))) {
StringBuilder appendNums = new StringBuilder();
appendNums.append(str.charAt(i));
for (int j = i+1; j < str.length(); j++) {
if (Character.isDigit(str.charAt(j))) {
appendNums.append(str.charAt(j));
} else {
break;
}
}
String appendNums2String = appendNums.toString();
total += Integer.parseInt(appendNums2String);
i += appendNums2String.length()-1;
}
}
return total;
}
The questions I have are:
- Are there any parts of this code you find to be poor practise or
poor quality? - Is it acceptable to append one digit before the (conditional) digits that proceed it, or should I be identifying the entire block
first, and then append? - What is a more concise way of solving this?
- For this question I relied heavily on Eclipse’s debugger until I got it right. Is this good or bad practise?
- Is it a better idea to use a
while
loop instead of the secondfor
loop, because the characters tested are relative toi
rather than thestring
‘s length? (I played around with this but couldn’t figure it out.)
Solution
Learning to use, and using the debugger are fantastic ideas. It is a valuable skill that will serve you well. Understanding how your code works (and breaks) is very valuable, and the debugger helps you through there.
The remainder of your specific questions are somewhat linked to your actual implementation, and I am going to suggest that your implementation would be simpler with a String split operation. The specification says:
Given a string, return the sum of the numbers appearing in the string,
ignoring all other characters. A number is a series of 1 or more digit
chars in a row.
Taking this literally, I would code this up as a Regular expresison system matching digits. Something like:
private static final Pattern NUMBERS = Pattern.compile("[0-9]+");
private static int digitSum(String s) {
Matcher matcher = NUMBERS.matcher(s);
int sum = 0;
while (matcher.find()) {
sum += Integer.parseInt(matcher.group());
}
return sum;
}
The Pattern
says “Look in the string for a sequence of digits”.
The method loops through all digit sequences, converts them to integers, and totals them.
I would advise using the Scanner class to do this for you.
private static int digitSum(String s) {
Scanner sc = new Scanner(s);
sc.useDelimiter("\D+"); // read numbers separated by non-numerics
int sum = 0;
while (sc.hasNextInt()) {
sum += sc.nextInt();
}
sc.close();
return sum;
}
If the numbers are particularly large, you can switch the scanner methods to hasNextBigInteger
and nextBigInteger
.
As for your code, I have a few comments on your naming
appendNums
doesn’t sound like a thing (an object), it sounds more like an action (method). I would change this toappendedDigitsBuffer
- numbers don’t belong in any name. Spell out
appendNums2String
and change it to a non verb (as per previous reason) toappendedDigits