current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

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();
}
share|improve this question
up vote 7 down vote accepted

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.

share|improve this answer
up vote 4 down vote

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.

share|improve this answer
1  
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‌​]); } – vnp Apr 11 at 19:12
    
True. Is it ok to edit my answer on behalf of this issue? – coderodde Apr 11 at 19:16
1  
@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 – janos♦ Apr 11 at 19:29
up vote 2 down vote

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[]).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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