6
\$\begingroup\$

Was just wanting some general feedback on the below code regarding
1. Readability
2. Structure
3. Efficiency

import java.util.Arrays;
import java.util.Collections; 
import java.util.List;
import java.util.Scanner;

/**
 * Created on 5/27/2016.
 *
 * Simple functions used to reverse either a sentence or a word.
 */


public class Reverse {

    /**
     * The main function is used to retrieve user input and apply it to one of the
     * two reverse functions.
     * @param args Not used in this application.
     */
    public static void main(String[] args){

        Scanner userInput = new Scanner(System.in);

        // Gets user input.
        System.out.print(">>> ");
        String input = userInput.nextLine();

        if(input.contains(" ")) // If contains space its a sentence
            System.out.println(reverse(Arrays.asList(input.split(" "))));
        else
            System.out.println(reverse(input));
    }

    /**
     * Used to reverse the order of given list.
     * @param words The list of words that will be reversed then returned.
     * @return The result of the reversed parameter.
     */
    private static String reverse(List<String> words){
        Collections.reverse(words); // Reverses order of words.
        return String.join(" ", words);
    }

    /**
     * Used to reverse the order of a single word.
     * @param word The string word that will be loaded into a list one character at a time.
     * @return The result of the reversed parameter.
     */
    private static String reverse(String word){
        List<String> letters = Arrays.asList(word.split("")); // Stores each letter of word into list.
        Collections.reverse(letters); // Reverses the order of the list.
        return String.join("", letters);
    }
}
\$\endgroup\$
6
\$\begingroup\$

It's unfortunate that there are two reverse() functions that do similar things. What is even more disconcerting is that they are inconsistent: main() is responsible for splitting a sentence into words, but reverse(List<String> words) joins them back up. On the other hand, reverse(String word) doesn't work that way: it does both the splitting and joining.

Why not solve both problems consistently with one function?

import java.util.*;
import java.util.regex.Pattern;

public class Reverse {
    private Reverse() {}  // Suppress the default constructor

    /**
     * Splits the input on the delimiter, reverses the list, then joins the
     * results with the delimiter.
     */
    public static String reverse(String s, String delimiter) {
        List<String> list = Arrays.asList(s.split(Pattern.quote(delimiter)));
        Collections.reverse(list);
        return String.join(delimiter, list);
    }

    public static void main(String[] args) {
        System.out.print(">>> ");
        String input = new Scanner(System.in).nextLine();

        if (input.contains(" ")) {
            // Reverse words in a sentence
            System.out.println(reverse(input, " "));
        } else {
            // Reverse characters in a word
            System.out.println(reverse(input, ""));
        }
    }
}

Note that there is a technicality, if reverse() is exposed as a public function as I have done. In that case, you would expect it to work for any delimiter, so the delimiter must be quoted such that String.split() treats it as a literal string rather than a regular expression.

\$\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.