1
\$\begingroup\$

Until now, all my command readers expect the entire command on a single line, so that it may be read with the call scanner.nextLine(). This code here caches the input line and after each Enter attempts to build command(s) from the currently stored lines. If no commands may be built, the previous lines remain in the line list, and the entire program expects more input.

Here is what I have:

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class CommandLineReader {

    private static final String COMMAND_SEPARATOR = ";";
    private static final String QUIT_COMMAND = "quit";
    private static final String PROMPT = "> ";

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        StringBuffer stringBuffer = new StringBuffer();
        List<String> lineList = new ArrayList<>();

        main_loop:
        while (true) {
            System.out.print(PROMPT);
            String inputLine = scanner.nextLine();
            lineList.add(inputLine);
            stringBuffer.delete(0, stringBuffer.length());

            for (String line : lineList) {
                stringBuffer.append(line.trim()).append(' ');
            }

            String bufferContent = stringBuffer.toString().trim();
            String[] bufferCommands = bufferContent.split(COMMAND_SEPARATOR);
            boolean lastCommandTerminated = 
                    bufferContent.endsWith(COMMAND_SEPARATOR);

            if (!lastCommandTerminated && bufferCommands.length < 2) {
                continue;
            }

            if (lastCommandTerminated) {
                for (String bufferCommand : bufferCommands) {
                    String command = bufferCommand.trim();
                    System.out.println(command + COMMAND_SEPARATOR);

                    if (command.equals(QUIT_COMMAND)) {
                        break main_loop;
                    }
                }

                lineList.clear();
            } else {
                for (int i = 0; i < bufferCommands.length - 1; ++i) {
                    String command = bufferCommands[i].trim();
                    System.out.println(command + COMMAND_SEPARATOR);

                    if (command.equals(QUIT_COMMAND)) {
                        break main_loop;
                    }
                }

                lineList.clear();
                // Return the last unfinished command:
                lineList.add(bufferCommands[bufferCommands.length - 1]); 
            }
        }

        System.out.println("Bye!");
        scanner.close();
    }
}

A possible session might look like this:


> yargh
> aye
> captain; abc; def; hello
yargh aye captain;
abc;
def;
> world
> ; quit;
hello world;
quit;
Bye!

My primary critique concern is usability: Can it be improved?

\$\endgroup\$
2
\$\begingroup\$

Here are some suggestions for improving the code:

  • Use a StringBuilder instead of a StringBuffer. StringBuilder is equivalent to StringBuffer, with the exception that it is not safe for multithreading, which makes it a bit faster. In your program, no other thread can ever have access to stringBuffer, because it is a local variable and you never pass it to a method or constructor, so you don't need synchronization on it.
  • Declare stringBuffer inside the while loop. It is not needed outside it and reset at the beginning of every loop iteration. So declaring it inside the loop reduces its scope to the smallest extent necessary, which makes the code clearer.
  • Replace this line:

    String bufferContent = stringBuffer.toString().trim();
    

    with this line:

    String bufferContent = stringBuffer.deleteCharAt(stringBuffer.length() - 1).toString();
    

    Since for (String line : lineList) will execute at least once (you manually added a line to lineList, so you can be sure it contains at least one element), stringBuffer is guaranteed to end with a single space character and have no whitespace directly preceding it, so the purpose of trim() is essentially to remove this single space character. The first line above, however, creates two String objects, one of which is immediately discarded after the line and only serves as an intermittent String to call trim() on, while the second line only creates one String object, that which is stored in bufferContent. Note that the second line will also modify stringBuffer, while the first line does not. However, since stringBuffer is never accessed again afterwards, this does not matter.

    In fact, an even more efficient approach would be to never let the final space character be appended in the first place by modifying the for (String line : lineList) loop to something like this:

    for (Iterator<String> lineListIterator = lineList.iterator(); lineListIterator.hasNext(); ) {
        stringBuffer.append(lineListIterator.next().trim());
        if (lineListIterator.hasNext()) {
            stringBuffer.append(' ');
        }
    }
    

    Then you can just call:

    String bufferContent = stringBuffer.toString();
    

    This might be more verbose than your original loop combined with the subsequent removal of the space character, but it is also more explicit, and thus maybe easier to read despite the increased verbosity.

  • Replace this construct:

    while (true) {
        // ...
        if (!lastCommandTerminated && bufferCommands.length < 2) {
            continue;
        }
    
        if (lastCommandTerminated) {
            // ...
        } else {
            // ...
        }
    }
    

    with this construct:

    while (true) {
        // ...
        if (lastCommandTerminated) {
            // ...
        } else if (bufferCommands.length >= 2) {
            // ...
        }
    }
    

    That way, you convey the exact same message, but with less code, and also, the while loop's control flow is easier to infer from the code structure inside it alone, because you have one continue statement less.

  • Exterminate duplicate code in the two if clauses by filtering out the common behavior. The only differences are that, in the second if clause, the last element of bufferCommands is not iterated over but added to lineList after lineList is cleared. A way to represent this logic more concisely might be replacing the two if clauses with one larger if clause:

    if (lastCommandTerminated || bufferCommands.length >= 2) {
        for (int i = 0;
                i < bufferCommands.length - (lastCommandTerminated ? 0 : 1);
                ++i) {
            String command = bufferCommands[i].trim();
            System.out.println(command + COMMAND_SEPARATOR);
    
            if (command.equals(QUIT_COMMAND)) {
                break main_loop;
            }
        }
    
        lineList.clear();
        if (!lastCommandTerminated) {
            lineList.add(bufferCommands[bufferCommands.length - 1]);
        }
    }
    
\$\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.