6
\$\begingroup\$

I was working on a suggestion for this question, but the method, I would've suggested the OP do something similar to, seems a bit convoluted.

It's supposed to take a String input and return an array of only the integers deliminated by spaces. It does this correctly, but It feels like I may be doing some unnecessary things or there's a more succinct way to do this, such as with regex.

public static int[] getIntegers(String s) {
    int[] result;
    ArrayList<Integer> helper = new ArrayList<>();
    StringBuilder sb = new StringBuilder();

    for (int i = 0; i < s.length(); i++) {
        if (s.charAt(i) == ' ' ^ i == s.length() - 1) {
            if (i == s.length() - 1) { sb.append(s.charAt(i)); }
            if (sb.toString().length() > 0) {
                try { helper.add(Integer.parseInt(sb.toString()));
                } catch(NumberFormatException nfe) {
                        // Ignore non-integers
                }
                sb.setLength(0);
                continue;
            }
        }
        sb.append(s.charAt(i));
    }

    result = new int[helper.size()];
    int i = 0;

    for (Integer n : helper) {
        result[i++] = n;
    }
    return result;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thinking this may be more appropriate for StackOverflow. \$\endgroup\$ – Legato Jan 7 '15 at 0:44
  • \$\begingroup\$ Would it be appropriate to tag this with rags-to-riches too? Not sure the extent of the makeover here. :D \$\endgroup\$ – h.j.k. Jan 7 '15 at 1:34
  • \$\begingroup\$ I personally wouldn't be opposed. Not just one but two superior methods! \$\endgroup\$ – Legato Jan 7 '15 at 2:09
10
\$\begingroup\$

Ohhhh.... XOR logic on booleans in Java ..... how esoteric:

if (s.charAt(i) == ' ' ^ i == s.length() - 1) {

That should be removed "just because". It's hard to understand, the logic is inverted, and it is not short-circuit, so it tests the i == s.length() - 1 even when s.charAt(i) == ' '

I think what you have is an inverted logic problem. You are looking for all the digits in the input, when, instead, you should be looking for not-digits....

The supporting logic, using StringBuilders, etc. is not a bad idea.... but, have you considered a regular-expression?

String[] digitwords = input.split("\\D+");
int[] result = new int[digitwords.length];
for (int i = 0; i < result.length; i++) {
    result[i] = Integer.parseInt(digitwords[i]);
}
return result;

The split there, is "split on all sequences that do not contain digits"

Java8 may help (with an added filter to remove empty values, not in the loop above), with:

return Arrays.stream(input.split("\\D+"))
       .filter(word -> !word.isEmpty())
       .mapToInt(Integer::parseInt)
       .toArray();
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Could you elaborate on what you mean by "short-circuit?" Isn't testing both i == s.length() - 1 and s.charAt(i) == ' ' the point? Is XOR really that uncommon? Thanks for the fantastic answer! It's exactly what I was looking for, I'm not too knowledgeable on using Regex, but I was sure there'd be a more eloquent way to do it with through that. The java 8 take is very well received. Both these methods get letters otherwise in between letters, possibly desirable behavior. Again, awesome answer! \$\endgroup\$ – Legato Jan 7 '15 at 1:13
  • 2
    \$\begingroup\$ Splitting on \D+ would obliterate negative signs. Splitting on whitespace is probably closer to the intended behaviour. \$\endgroup\$ – 200_success Jan 7 '15 at 3:03
  • 1
    \$\begingroup\$ You can simplify the third line of your nice Java 8 example to: .mapToInt(Integer::parseInt) \$\endgroup\$ – Alice Purcell Jul 24 '15 at 13:44
  • 2
    \$\begingroup\$ @chrispy - of course. It's a bad habit I got in to when I first started playing with Java8, and it's taking me a while to undo that.... in this case, I wrote that at my worst (but I still do it too often). It looks more natural to me for some reason to see the arguments passed to the function. Still, it's not right, you are. \$\endgroup\$ – rolfl Jul 24 '15 at 13:54
4
\$\begingroup\$

Well, @rolfl's is nearly identical to mine, and it's a lot simpler.

If you wanted to use your approach, here's how I would tweak it:

public static int[] getIntegers(String s) {
    ArrayList<Integer> helper = new ArrayList<>();
    StringBuilder sb = new StringBuilder();

    for (int i = 0; i <= s.length(); i++) {
        if (i == s.length() || s.charAt(i) == ' ') {    // End or delimiter
            try {
                helper.add(Integer.parseInt(sb.toString()));
            } catch (NumberFormatException discardNonIntegers) {
            }
            sb.setLength(0);
        } else {                                        // Possible number
            sb.append(s.charAt(i));
        }
    }

    int[] result = new int[helper.size()];
    int i = 0;
    for (int n : helper) {
        result[i++] = n;
    }
    return result;
}

Changes to note:

  • int[] result is declared where it is assigned.
  • The loop terminates at a fictitious index just past the end of the string, to eliminate the special case if (i == s.length() - 1) { sb.append(s.charAt(i)); }
  • if (i == s.length() || s.charAt(i) == ' ') is easier to understand than your XOR, I think.
  • It is not necessary to check if (sb.toString().length() > 0), as trying to parse an integer from an empty string would just throw an exception anyway. If you did want to check, then if (sb.length() > 0) would work.
  • You have to choose a name for NumberFormatException, so you might as well make it meaningful.
  • I find if/else easier to understand than if-continue. Due to the parallelism in formatting, it's easier to see that on each iteration of the loop we are either going to clear the buffer or append a character.
  • Nobody likes boxed types. You might as well unbox n slightly sooner.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yeah, come to think of it the continue was no longer necessary(I tried something else before and it was a remnant), Thank you for the 'kindred to mine' approach! It's insightful. \$\endgroup\$ – Legato Jan 7 '15 at 18:01

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.