Reversing only vowels in a string

Question

Given a string, reverse only the vowels present in it and print the resulting string. example input: ransomware output: rensamwora

Here is my solution:

import java.util.Scanner;

public class ReversingVowels {
    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        int numberOfTestCases = scanner.nextInt();

        while (numberOfTestCases --> 0) {
            StringBuilder stringWhoseVowelsShouldBeReversed = new StringBuilder(scanner.next());
            System.out.println(reverseVowels(stringWhoseVowelsShouldBeReversed));
        }

        scanner.close();
    }

    private static String reverseVowels(StringBuilder string) {

        if (string.length() == 0 || string.length() == 1) return string.toString();

        int start = 0;
        int end = string.length() - 1;

        while (true) {
            while (! isVowel(string.charAt(start))) {
                start++;
            } 
            while (! isVowel(string.charAt(end))) {
                end--;
            }
            if (start >= end) {
                break;
            }
            else {
                swapVowels(string, start, end);
                start++;
                end--;
            }
        }
        return string.toString();
    }

    private static boolean isVowel(char character) {
        switch(character) {
        case 'a':
        case 'e':
        case 'i':
        case 'o':
        case 'u': return true;
        default: return false;
        }
    }

    private static void swapVowels(StringBuilder string, int index1, int index2) {
        char tmp = string.charAt(index1);
        string.setCharAt(index1, string.charAt(index2));
        string.setCharAt(index2, tmp);
    }
}

Questions:

1. Are there any bad practises
2. Are there any issues
3. Can this be improved

Answer 1

1) No more than one instruction per line.

if (string.length() == 0 || string.length() == 1) return string.toString();

i.e.:

if (string.length() == 0 || string.length() == 1) {
    return string.toString();
}
// or, better:
// if (string.length() < 2) {
//     return string.toString();     
// }

2) Your main while feels strange, I don't like this creative use of postdecrement and greater then operator

while (numberOfTestCases --> 0)  // BAD! See https://stackoverflow.com/questions/1642028/what-is-the-operator-in-c

In this case a for loop is better because you know the number of iterations.

3) while (true) is bad and force the developer to read all the code to understand how to break the loop

4) No need to pass a StringBuilder to reverseVowels: the method itself should create it.

5) Use temporary variables to improve readability:

...
for (int i = 0; i < numberOfTestCases; i++) {
    String input = scanner.nextLine();

    String vowelsReversedString = reverseVowels(input);

    System.out.println("OUTPUT: " + vowelsReversedString);
}  
...

...
private static String reverseVowels(String string) {
    if (string == null || string.length() < 2) {
        return string;
    }

    StringBuilder sb = new StringBuilder(string);

    int start = 0;
    int end = sb.length() - 1;

    while (start < end) {
        ...
    }

    return sb.toString();
}
...

Answer 2

Rather than juggling start, end and charAt() you should instead iterate over the characters in the string. This should also remove the need for the initial length check.

Why do you pass the number of test cases on standard input? Why not just process standard input while scanner returns something?

The class name should probably be something like VowelReverser, and the methods should not be static.

I'm not familiar with StringBuilder, but it seems that it would be more natural to pass around a String instead.

Answer 3

You don't need to check for input lengths of 0 or 1; your initial values of "start" and "end" will catch that: end <= start in either case.

@heijenoort's reworking of the while loop condition is the same as mine. Better to avoid break in the middle of loops if you can, and this one is easy to avoid.

You are returning a String so for symmetry it is better to accept a String as parameter and convert to a StringBuilder internally. You can change that if you have already done the conversion elsewhere in your code. In that case consider returning a StringBuilder instead, so input and output are the same class. That avoids possible future errors from an invisible change of class, hidden behind a method call.

I find it confusing that you call a StringBuilder variable "string", better to call it something like "text" or "sbText".

Your code assumes that there is at least one vowel in the supplied text. Currently it will fail on an input of "zzzzzzz" as the indexes will fall off the ends of the array. If your input guarantees at least one vowel, then that is not a problem. If it does not, then you will need to fix it.

Your "isVowel()" method does not recognise A, E, I O, U as vowels. You know more about the expected input than I do, so this may not be an error. You can always use toLowerCase() on the character before checking.

My partial code, with some comments:

private static String reverseVowels(String text) {

    StringBuilder sbText = new StringBuilder(text)

    int start = 0;
    int end = sbText.length() - 1;

    while (start < end) {

        // Step start index to next vowel.
        while (! isVowel(sbText.charAt(start))) {
            start++;
        } 

        // Step end index to previous vowel.
        while (! isVowel(sbText.charAt(end))) {
            end--;
        }

        // Swap vowels if indexes have not met or crossed.
        if (start < end) {
            swapVowels(sbText, start, end);
            start++;
            end--;
        }

    } // end while

    return sbText.toString();
}