Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'd like this code to be improved

Input string: codereview is awesome

Output string: awesome is codereview

My approach:

  1. Reverse the entire string
  2. Reverse each word in a string

package com.arun.reversewords;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class ReverseWords {

public static void main(String[] args) throws IOException {

    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

    System.out.println("Enter the string");
    String s = br.readLine();

    String rev = StringRev(s);

    System.out.println(rev);

}

private static String StringRev(String s) {

    char[] modString = new char[s.length()];

    for (int i = 0; i < s.length(); i++) {
        modString[i] = s.charAt(s.length() - 1 - i);
    }

    s = s.copyValueOf(modString);
    String reverseWord = "";
    String eachWord;
    for (String part : s.split(" ")) {
        eachWord = new StringBuilder(part).reverse().toString();
        reverseWord = reverseWord + eachWord + " ";
    }

    return reverseWord;

  }

}
share|improve this question

5 Answers 5

up vote 17 down vote accepted

There are several problems with your approach:

  • It's strange to reverse the letters, reorder the words, and then reverse the letters again. It's inefficient and unnecessary. You could just reverse the words.
  • The string you return has a trailing space: "awesome is codereview "
  • The naming standard is to use camelCase for methods, which "StringRev" violates, and it's not a good name anyway ("reverseWords" would be better)
  • s for the sentence to reverse is not a good name

It would be more efficient to split by space and then join the words in reverse order. Also keep in mind that String concatenation is not efficient. The efficient way is to use a StringBuilder.

This implementation is more efficient:

public static String reverseWords(String sentence) {
    String[] parts = sentence.split(" ");

    StringBuilder builder = new StringBuilder();
    builder.append(parts[parts.length - 1]);

    for (int i = parts.length - 2; i >= 0; --i) {
        builder.append(" ").append(parts[i]);
    }

    return builder.toString();
}

To verify that the method works correctly, it's good to have unit tests:

@Test
public void testEmptyString() {
    assertEquals("", ReverseWords.reverseWords(""));
}

@Test
public void testWithSingleWord() {
    assertEquals("awesome", ReverseWords.reverseWords("awesome"));
}

@Test
public void testWithTwoWords() {
    assertEquals("is awesome", ReverseWords.reverseWords("awesome is"));
}

@Test
public void testWithSentence() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("codereview is awesome"));
}

It wasn't part of your specifications, but I would go a bit further and make the solution more robust, by making it work with leading or trailing spaces, and multiple spaces in between words:

public static String reverseWords(String sentence) {
    // trim leading and trailing spaces
    // split on 1 or more spaces between words
    String[] parts = sentence.trim().split("\\s+");
    // ...
}

@Test
public void testWithLeadingSpaces() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("  codereview is awesome"));
}

@Test
public void testWithMultipleSpaces() {
    assertEquals("awesome is codereview", ReverseWords.reverseWords("codereview   is awesome"));
}

Your program reads a single sentence, reverses the words and prints the output. A BufferedReader can be efficient for processing large inputs, but for such simple uses, a Scanner would be easier and simpler, like this:

public static void main(String[] args) {
    System.out.println("Enter the string");
    Scanner scanner = new Scanner(System.in);
    System.out.println(reverseWords(scanner.nextLine()));
}

With this solution you don't need to declare the method to throw an exception.

For extra user-friendliness, you might want to allow input as command line arguments, and fall back to using System.in only when no arguments were given:

public static void main(String[] args) {
    if (args.length == 0) {
        System.out.println("Enter the string: ");
        Scanner scanner = new Scanner(System.in);
        System.out.println(reverseWords(scanner.nextLine()));
    } else {
        for (String arg : args) {
            System.out.println(reverseWords(arg));
        }
    }
}
share|improve this answer
5  
Unit tests, me like! –  Simon André Forsberg Aug 24 at 13:07
    
Can you expand on "...String concatenation is not efficient. The efficient way is to use a StringBuilder." That's something I've been wondering recently but haven't been able to find a good source on –  musher Aug 25 at 14:54
    
@musher see this article: jtechies.blogspot.fr/2012/07/… –  janos Aug 25 at 15:13
    
Interesting read, thanks! –  musher Aug 25 at 16:17

You have a compiler warning in your code:

The static method copyValueOf(char[]) from the type String should be accessed in a static way

This is because it should be:

s = String.copyValueOf(modString);

In your main method, instead of doing:

String rev = StringRev(s);

System.out.println(rev);

You can do both the output and the reversing on the same line:

System.out.println(StringRev(s));

private static String StringRev(String s) {
  • Avoid short variable names like s. A better option would be input
  • Method names in Java should start with a lowercase letter, like stringRev
  • A better method name would be reverseWords as you're not really reversing the string and it's not obvious what Rev is short for.

reverseWord = reverseWord + eachWord + " ";

  • Your output will always end with a trailing space, this is not optimal.
  • You are using String concatenation by adding a string to a string, this will create multiple string objects. A more efficient approach is to use StringBuilder

My approach :

  1. Reverse the entire string
  2. Reverse each word in a string

This is inefficient as you are reversing twice. A better option would be to first separate the words, and then reverse the words, not the content of the words.

private static String reverseWords(String input) {

    String[] splitted = input.split(" ");
    StringBuilder output = new StringBuilder();

    for (int i = 0; i < splitted.length; i++) {
        if (output.length() > 0) {
            output.append(" ");
        }
        output.append(splitted[splitted.length - 1 - i]);
    }

    return output.toString();
}

Note that by checking for output.length() > 0 it's possible to deal with the trailing space problem.


Another alternative

Using some Java magic of converting an array to a list, reversing a list, and the new method in Java 8 for joining strings, we can do this:

private static String reverseWords(String input) {
    String[] splitted = input.split(" ");
    List<String> list = Arrays.asList(splitted);
    Collections.reverse(list);
    return String.join(" ", list);
}

I would however recommend that you do it the old-fashioned way, so that you better understand what your code is doing. I just wanted to provide this approach so that you know what you have to look forward to :)

share|improve this answer
    
Note that testing "output.length() > 0" works inconsitently. It removes trailing spaces, but not trailing spaces. "..a.b.c.." gives "c.b.a.." (where . represents a space for readability). –  Florian F Aug 25 at 19:01

Your question is a classic question in C programming and the solution to reverse the string and then reverse the individual words back is the trick that allows for a simple and efficient solution. The point of this method is that it allows to reverse a sentence in an array of chars without any other storage than the input string.

But if you implemement it by splitting the string with split(), reverse the substrings and collect these back to a String, you loose the benefit of the method, and might as well just split the string and collect the words backwards.

You should either implement this method and work at the character level, or go for the slower but simpler method and just split the words and collect them backwards in a StringBuilder.

Reversing the string and reverse the words

You can use this method if you need efficency and want to minimize variable allocation. But you need to work on an array of characters. Here is how it could work.

/** reverse the order of the characters in an array from start to end-1 */
private static void reverse(char[] array, int start, int end){
    int i = start;
    int j = end-1;
    while( i < j ){
        char ch = array[i];
        array[i] = array[j];
        array[j] = ch;
        ++i;
        --j;
    }
}

/** Reverse the order of words in a String */
private static String reverseString(String s) {

    char[] modString = s.toCharArray();
    // reverse the whole string
    reverse(modString, 0, modString.length);
    // reverse each word back
    int begin = 0;
    for( int i=0 ; i<modString.length ; i++ ){
        if( modString[i]==' ' ){
            reverse(modString, begin, i);
            begin = i+1;
        }
    }
    reverse(modString, begin, modString.length);
    // return the result
    return new String(modString);
}

Reverse words using String manipulation

There are few things wrong with your code.

  • the procedure name should start with a lowercase
  • if you split your string anyway, reverse only the order of the words
  • the resulting string should be built in a StringBuilder, not a String
  • you are actually appending a space to the end in the result
  • comment your code

Here is an example that addresses all of these

/** Reverse the order of words separated by a space in a String */
private static String stringRev(String s) {
    // split the sentence at every space
    String[] words = s.split(" ");
    // join the words backwards
    StringBuilder reversed = new StringBuilder(s.length());
    for( int i=words.length-1 ; i>=0 ; --i ){
        reversed.append(words[i]);
        if( i>0 ){
            reversed.append(" ");
        }
    }
    // return the reversed sentence
    return reversed.toString();
}
share|improve this answer
1  
Welcome to Code Review! This is a nice first answer! Excellent point that the reversing-twice approach can be used to minimize storage. –  Simon André Forsberg Aug 25 at 0:21
1  
I would not add those comments though as they only explain what the code is doing and not why it's doing it. It's often easier to read the code than to read it's comments to figure out what it does. (With the exception of esoteric programming languages of course) –  Simon André Forsberg Aug 25 at 0:22
    
OK, maybe I over-comment. I would leave the comment "join the words backwards" though. –  Florian F Aug 25 at 9:21

Your Approach

  • Variable names: reverseWord doesn't really match what it is. I used reversedSentence which I think fits better (your method StringRev could also be named reverseSentence)
  • reversing a string: you use two different approaches: StringBuilder.reverse and your modString approach. Why are you using two different solutions for the same problem? I would extract it to a new method: reverseString and use that method in both places

If you do that, your code would look like this:

private static String reverseSentence(String sentence) {
    sentence = reverseString(sentence);

    String reversedSentence = "";
    for (String word : sentence.split(" ")) {
        reversedSentence += reverseString(word) + " ";
    }

    return reversedSentence;    
  }

private static String reverseString(String string) {
    return new StringBuilder(string).reverse().toString();
}

Which I think is a lot easier to understand. If you care about performance, you can use a StringBuilder instead of + and +=.

A different Approach

I think that your approach is way too complex, there is no need to reverse the whole string first. I would just simply reverse the words:

private static String reverseSentence(String sentence) {
    String reversedSentence = "";
    for (String word : sentence.split(" ")) {
        reversedSentence = word + " " + reversedSentence;
    }
    return reversedSentence;
}

It's easier to understand and performs a lot better (and again, you can use StringBuilder for even better performance).

share|improve this answer
4  
I think your first solution leaves a trailing space. –  nhgrif Aug 24 at 13:05
3  
@nhgrif it does, but so does the code of the op (I only extracted the reverseString code and renamed some variables). I just assumed that it doesn't matter to them (but your right, it is an error). –  tim Aug 24 at 13:07

I think you can simply try this:-

String str = "your input source string";
String strArray[]=str.split(" ");
  for(int i=strArray.length()-1;i>=0;i--) 
      System.out.print(strArray[i]+" ");
share|improve this answer
    
Not a real answer, Need to clarify and describe more. –  JaDogg Oct 1 at 6:37

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.