[Java] Please break / tune my code

[tinny]

Hi all

(I have coders block )

I am looking to write a robust generic command line interface class for Java. I have hacked together something that is working quite well. I am hoping that I can get a few people to try different commands and see how well it works on machines other than my own. Also suggestions on how the code could be tuned are appreciated.

Im not 100% happy with the “processErrorMessage” and “processResultMessage” methods.

Who knows, maybe it works well and people will actually like it...?

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

public class CLIProcess implements Runnable {

    private static final int INIT_WAIT = 1000;
    private static final int STREAM_WAIT = 1000;
    public static final int IN_PROGRESS = -1;
    private boolean running;
    private int result = IN_PROGRESS;
    private String command;
    private String errorMessage;
    private String successMessage;
    private Process process;

    public CLIProcess(String command) {
        this.command = command;
        running = false;
    }

    public void run() {
        running = true;

        try {
            Runtime runTime = Runtime.getRuntime();
            process = runTime.exec(command);

            try {
                Thread.sleep(INIT_WAIT);
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }

            errorMessage = processStream(process.getErrorStream());
            successMessage = processStream(process.getInputStream());

            result = process.waitFor();
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        } catch (IOException ex) {
            ex.printStackTrace();
        } finally {
            running = false;
        }
    }
    
    private String processStream(InputStream is) throws IOException {
        BufferedReader br = new BufferedReader(new InputStreamReader(is));
        StringBuilder bs = new StringBuilder();
        
        while (br.ready()) {
            bs.append(br.readLine());
            try {
                Thread.sleep(STREAM_WAIT);
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        }
        return bs.toString().trim();
    }
    
    public int getResult() {
        return result;
    }
    
    public boolean isRunning() {
        return running;
    }
    
    public String getErrorMessage() {
        return errorMessage;
    }
    
    public String getSuccessMessage() {
        return successMessage;
    }
}

public class Main {

    public static void main(String[] args) {
        try {
            CLIProcess pro = new CLIProcess("gksu apt-get update");
            Thread thread = new Thread(pro);
            thread.start();
            Thread.sleep(500);
            while (pro.isRunning()) {
                Thread.sleep(500);
            }

            System.out.println("Result Code: " + pro.getResult());
            System.out.println("Result: " + pro.getSuccessMessage());
            System.out.println("Error: " + pro.getErrorMessage());
        } catch (InterruptedException ex) {
               ex.printStackTrace();
        }
    }
}

Cheers

[txcrackers]

I have one very simple question: what the heck are you sleeping for?

I'd go back and re-look at ProcessBuilder and think more about multi-threading instead of a single thread (possibly even nio).

[Quikee]

processErrorMessage() and processResultMessage() do exactly the SAME thing!

[tinny]

I have one very simple question: what the heck are you sleeping for?

I'd go back and re-look at ProcessBuilder and think more about multi-threading instead of a single thread (possibly even nio).

Have you run the code? Did it work? Try setting the sleeps to 0 and see what happens.

Also it is a multi-threaded application have a look at the main method. You have your main method of execution and a "CLIProcess" thread.

Ill have a look at ProcessBuilder.

[ceclauson]

Hmmm...interesting problem

I went ahead and took a crack at it. Here's my solution (I tried it and it seems to work):

public class CLIProcess implements Runnable {

	//the command associated with this CLIProcess
	private final String command;
	//whether or not the process is running
	private boolean processIsRunning = false;
	//the result of the last process that ran;
	private int result;

	public CLIProcess(String command) {
		this.command = command;
	}

	//synchronized so than only one thread can be
	//in this method at a time
	public synchronized void run() {
		//start the process executing the command
		final Process process = Runtime.getRuntime().exec(command);
		//and record that this is being done
		processIsRunning = true;

		//references to the error and output streams of the process
		//stream representing standard error
		final InputStream errorStream = process.getErrorStream();
		//confusingly, stream representing standard out
		final InputStream outputStream = process.getInputStream();
		//create a new thread whose sole purpose in life is to
		//flush the output stream and error stream to standard
		//out and standard error
		new Thread(new Runnable() {
			void run() {
				//thread simply keeps flushing while
				//the process is running
				while(processIsRunning) {
					flushStreamToError(errorStream);
					flushStreamToOutput(outputStream);
				}
				//when process ends, do one more flush
				flushStreamToError(errorStream);
				flushStreamToOutput(outputStream);
				//and then die
			}
		}).start();
		
	private static void flushStreamToError(InputStream is) {
		while((int i = is.read) != -1)
			System.err.write((byte) i);

	}

	private static void flushStreamToOutput(InputStream is
		while((int i = is.read) != -1)
			System.out.write((byte) i);
	}
}

This class is similar to yours in that it runs the command. But it's different in that instead of saving the command output, it prints it to standard out and standard error in real time. The thread that enters the run method "splits" -- one thread monitors the output of the process and prints it as it comes, and the other monitors the process to see if it's finished. Once the process is finished, the second thread notifies the first thread of this, which dies, and then the second thread (i.e., the original thread) exits.

However, if you're not interested in getting the output in real time, but caching it for later retrieval, which it seems you're interested in doing, you could easily do this with just one thread. Just have the thread wait on the process, and then when the process is over, do all of the flushing of the output and error at the end.

Again, it's an interesting problem. Hopefully you find this post helpful.

Cooper

[tinny]

processErrorMessage() and processResultMessage() do exactly the SAME thing!

Yeah, they are pretty much the same.

They do read different streams and build different Strings.

Ive created a generic "processStream" method.

See my edited original post.

Of course none of this makes my code more robust or helps me know what other commands run / dont run with this code.

[tinny]

Thanks for giving it a crack ceclauson.

I think you may have a cut and paste error in your code?

[drubin]

May I ask why you would want to do this in java?

[Diabolis]

May I ask why you would want to do this in java?

Might be handy for Java applications that use other programs written in different languages. Probably I use some of this code, of course I'll post back If something does not work.

[drubin]

I have an idea That I have from some old code that I wrote a while back...

Will post some of it when i get home.