6
\$\begingroup\$

Is there a better, more efficient or nicer way to do this?

public static String sortString(String s){

    String[] strArray= s.split("\\s+");
    Arrays.sort(strArray);
    StringBuilder sb = new StringBuilder();
    for (int i=0; i<strArray.length; i++){
        sb.append(strArray[i]);
        sb.append(" ");
    }
    return sb.toString();
}
\$\endgroup\$
7
\$\begingroup\$

A couple of comments:

  • String[] strArray = s.split("\\s+"); will create a Pattern object each time this is called, which might hurt the performance. It would be better to reuse the same pattern. Declare it as a constant:

    private static final Pattern PATTERN = Pattern.compile("\\s+");
    

    and then you can use

    String[] strArray = PATTERN.split(s);
    
  • Use a for-each loop instead of a loop using index. This is both easier to read and to maintain when you don't need the index. So instead of having

    for (int i=0; i<strArray.length; i++){
        sb.append(strArray[i]);
    

    you can have

    for (String str : strArray){
        sb.append(str);
    

If you can use Java 8, you could have

public static String sortString(String s){
    return PATTERN.splitAsStream(s).sorted().collect(Collectors.joining(" "));
}

This splits the input String into a Stream<String> using splitAsStream. Then the stream is sorted and finally collected with a collector joining all elements together, separated by a space, using Collectors.joining(" "). As a side-note, this will not include the last white-space at the end of the joined Strings, unlike the code in your question.

\$\endgroup\$
4
\$\begingroup\$

You could pass the length of the source string to the StringBuilder:

StringBuilder sb = new StringBuilder(s.length());

That way, you make sure that the builder does not have to resize in order to accommodate the string, and that you don't waste memory.

Also, I would fix a little bit (taking the advice by vnp):

sb.append(strArray[0]);

for (int i = 1; i < strArray.length; ++i) {
    sb.append(" ").append(strArray[i]);
}

..., so that you do not conclude the last word with a space.

Hope that helps.

\$\endgroup\$
  • 1
    \$\begingroup\$ Testing for such condition at each iteration hurts performance. Recommended way is to sb.append(strArray[0]); for (i = 1; i < strArray.length; ++i) { sb.append(" "); sb.append(strArray[i]); } \$\endgroup\$ – vnp Apr 11 '16 at 19:12
  • \$\begingroup\$ True. Is it ok to edit my answer on behalf of this issue? \$\endgroup\$ – coderodde Apr 11 '16 at 19:16
  • 1
    \$\begingroup\$ @coderodde yes it's ok to improve (even rewrite) your answer based on comments you receive. When doing so, it's recommended to credit vnp for the tip \$\endgroup\$ – janos Apr 11 '16 at 19:29
2
\$\begingroup\$

Here's one which removes the entirety of the StringBuilder operations you do. If on Java 8, simply use String.join(CharSequence delimiter, CharSequence... elements). In your case, delimiter would be " ", and elements would be strArray.

Final Code:

public static String sortString(String s){
    String[] strArray = s.split("\\s+");
    Arrays.sort(strArray);
    return String.join(" ", strArray);
}

(A Java 8 3-liner solution, possibly a bit less complex than the one proposed by @Tunaki - 3 method calls here to 4 there).

And, since we are on Java 8 already, and you are concerned about performance and have really long 40-50 word Strings, try Arrays.parallelSort(Object[]) in place of Arrays.sort(Object[]).

\$\endgroup\$

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.