Code Review Request

[pitoniakm]

folks,

I modified this class on executing system commands from a version on
koders.com. Being a less than expert coder I was wondering if I could
get some code inspection/style tips to insure it is coded properly.
please just ignore the imported service libs i use.

thx in advance for any assistance,

mp

/*
* Copyright (C) 2002 by Michael Pitoniak ([email protected]).
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to:
*
* The Free Software Foundation, Inc.,
* 59 Temple Place - Suite 330
* Boston, MA 02111-1307
* USA
*/

//reference When Runtime_exec() won't.htm in the docs directory for
details

package harness.utils.systemServices;


import harness.common.CommonConstants;
import harness.utils.classServices.ClassServices;
import harness.utils.exceptionServices.ExceptionServices;
import harness.utils.systemServices.interfaces.SysExecCmdInterface;
import harness.utils.timeServices.TimeServices;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.iutputStream;
import java.io.PrintWriter;
import java.net.InetAddress;



public class ExecSysCmdThread2 extends java.lang.Thread implements
SysExecCmdInterface, CommonConstants {
private String m_CmdStr = null;
private String m_RedirectFile = null;
private long m_TimeOutMS;
private String m_StdOutResponse = null;
private String m_StdErrResponse = null;
private StringBuffer m_ExceptionBuffer = null;
private String m_Response = null;
private Runtime m_Runtime = null;
private Process m_Process = null;
private String m_InetAddrName = null;

public ExecSysCmdThread2(){
try{
m_Runtime = Runtime.getRuntime();
m_InetAddrName = InetAddress.getLocalHost().getHostName();
if(m_InetAddrName == null){
m_InetAddrName = DEFAULT_REMOTE_NAME;
}
}catch(Exception e){
e.printStackTrace();
}
}

public ExecSysCmdThread2(String cmd, long streamReaderTimeOutMs) {
this();
m_CmdStr = cmd;
m_TimeOutMS = streamReaderTimeOutMs;
}

public void run() {
executeSystemCommand(m_CmdStr, null, m_TimeOutMS);
}

/*
* NOTE: on windows cmd paths that contain spaces must be quoted:
* ex) "\"c:/program files/windows/notepad.exe\""
*/
private String executeSystemCommand(String cmd, String
redirectFile, long streamReaderTimeOutMs){
m_CmdStr = cmd;
m_RedirectFile = redirectFile;
m_TimeOutMS = streamReaderTimeOutMs;

FileOutputStream fos = null;
InputStream is = null;
InputStream es = null;
OutputStream os = null;
StreamReaderThread stdOutStreamReaderThread = null;
StreamReaderThread stdErrStreamReaderThread = null;
int exitValue;

try{
m_ExceptionBuffer = new StringBuffer();

if(redirectFile != null){
fos = new FileOutputStream(redirectFile, false);
}
//System Output Neccessary to track progress
System.out.println(TimeServices.getTimeAsString() + "
Executing: " + cmd + " StreamReader TimeOutMS: " + m_TimeOutMS +
NEWLINE_CHAR);
m_Process = m_Runtime.exec(cmd);

//System.out.println(m_Runtime.freeMemory());

/*It is important to note that the method used to obtain a
process's output stream is called getInputStream(). The
thing
to remember is that the API sees things from the
perspective
of the Java program and not the external process.
Therefore,
the external program's output is the Java program's input.
And that logic carries over to the external program's
input
stream, which is an output stream to the Java program.
*/
is = m_Process.getInputStream();
es = m_Process.getErrorStream();
os = m_Process.getOutputStream();

stdOutStreamReaderThread = new StreamReaderThread(is,
STDOUT_PREFIX);
stdErrStreamReaderThread = new StreamReaderThread(es,
STDERR_PREFIX);

stdOutStreamReaderThread.start();
stdErrStreamReaderThread.start();

//join(0) means infinite
stdOutStreamReaderThread.join();
stdErrStreamReaderThread.join();

m_StdOutResponse =
stdOutStreamReaderThread.getResponseBuffer();
m_StdErrResponse =
stdErrStreamReaderThread.getResponseBuffer();

if(stdOutStreamReaderThread.isAlive() |
stdErrStreamReaderThread.isAlive()){
m_StdErrResponse = m_StdErrResponse.concat(NEWLINE_CHAR
+ m_InetAddrName + "->" + ClassServices.getCurrentMethodName() +
"************ ExecSysCmdThread StreamReader TIMEOUT ************");
}
/* Process.waitFor() causes the current thread to wait, if
necessary, until the
* process represented by this Process object has
terminated. This method returns
* immediately if the subprocess has already terminated. If
the subprocess has not
* yet terminated, the calling thread will be blocked until
the subprocess exits.
*
* Returns:
* the exit value of the process. By convention, 0
indicates normal termination.
* */
exitValue = m_Process.waitFor();
//m_StdOutBuffer.append("ExitValue: " + exitValue +
NEWLINE_CHAR);
}catch(Exception e){
m_ExceptionBuffer.append(NEWLINE_CHAR +
ClassServices.getCurrentMethodName() +
ExceptionServices.getStackTrace(e));
}finally{
try{
if(is != null){
is.close();
is = null;
}
}catch(Exception e){}
try{
if(es != null){
es.close();
es = null;
}
}catch(Exception e){}
try{
if(os != null){
os.close();
os = null;
}
}catch(Exception e){}
try{
if(fos != null){
fos.close();
fos = null;
}
}catch(Exception e){}
try{
if(m_Process != null){
m_Process.destroy();
m_Process = null;
}
}catch(Exception e){}
}
//collect StreamReaders ExceptionBuffers

m_ExceptionBuffer.append(stdOutStreamReaderThread.getExceptionBuffer()
+ NEWLINE_CHAR + stdErrStreamReaderThread.getExceptionBuffer());

//NOTE: executeSystemCommand() returns "BOTH" StdOut, and
StdErr data
//remove the final \n as writeOutput() will add that
m_Response = m_StdOutResponse.concat(NEWLINE_CHAR +
m_StdErrResponse).trim();

return m_Response;
}

public String getStdOutStdErrConcatResponse(){
return m_StdOutResponse.concat(NEWLINE_CHAR +
m_StdErrResponse).trim();
}

public String getStdOutResponse() {
return m_StdOutResponse;
}

public String getStdErrResponse() {
return m_StdErrResponse;
}

//The ExceptionBuffer contains all ExecSysCmdThread and
StreamReader Exception data
public String getExceptionBuffer() {
return m_ExceptionBuffer.toString().trim();
}

/* note: why am I not using BufferedReaders? I am not 100% sure in
this,
but I believe that BufferedReader will block if it does not see
end-of-line character in the data. If process writes a long string
of
data (longer than buffer) without any newline characters, process
will
block, too. So, we deadlock again. I don't care about performance
(what performance? starting external process) or ends of line, so
I am
just reading both streams byte by byte.*/
private class StreamReaderThread extends Thread{
InputStream m_InputStream = null;
String m_LinePrefix = null;
OutputStream m_RedirectOutputStream = null;
InputStreamReader m_InputStreamReader = null;
StringBuffer m_StreamReaderResponseBuffer = null;
StringBuffer m_StreamReaderExceptionBuffer = null;

public StreamReaderThread(InputStream inputStream, String
prefix){
m_InputStream = inputStream;
m_LinePrefix = prefix;

m_StreamReaderResponseBuffer = new StringBuffer();
m_StreamReaderExceptionBuffer = new StringBuffer();
}

public StreamReaderThread(InputStream inputStream, String
prefix, OutputStream redirectOutputStream){
this(inputStream, prefix);
m_RedirectOutputStream = redirectOutputStream;
}

public void run(){
PrintWriter redirectedFilePrintWriter = null;
boolean bFirstPass = true;
int ch;

try{
m_InputStreamReader = new
InputStreamReader(m_InputStream);
//this is for redirected files
if (m_RedirectOutputStream != null){
redirectedFilePrintWriter = new
PrintWriter(m_RedirectOutputStream);
}

while(-1 != (ch = m_InputStreamReader.read())){
if(bFirstPass){
appendResponseBuffer(m_LinePrefix);
bFirstPass = false;
}
appendResponseBuffer((char)ch);
//start of a new line, add prefix
if((char)ch == '\n'){
appendResponseBuffer(m_LinePrefix);
}
//handle redirected streams
if (redirectedFilePrintWriter != null){
redirectedFilePrintWriter.print(ch);
}
}
if (redirectedFilePrintWriter != null){
redirectedFilePrintWriter.flush();
}
}catch (Exception e){

m_StreamReaderExceptionBuffer.append("\nStreamReaderThread Read
error:"+ e.getMessage());
}finally{
//NOTE: it is safest to close most "raw" stream
try{
if(m_InputStream != null){
m_InputStream.close();
m_InputStream = null;
}
}catch(Exception e){

m_StreamReaderExceptionBuffer.append("\nStreamReaderThread Read
error:"+ e.getMessage());
}
try{
if(redirectedFilePrintWriter != null){
redirectedFilePrintWriter.close();
redirectedFilePrintWriter = null;
}
}catch(Exception e){

m_StreamReaderExceptionBuffer.append("\nStreamReaderThread Read
error:"+ e.getMessage());
}
}
}

private synchronized void appendResponseBuffer(String str){
m_StreamReaderResponseBuffer.append(str);
}

private synchronized void appendResponseBuffer(char c){
m_StreamReaderResponseBuffer.append(c);
}

private synchronized String getResponseBuffer(){
return m_StreamReaderResponseBuffer.toString();
}

private synchronized String getExceptionBuffer(){
return m_StreamReaderExceptionBuffer.toString();
}

}

public static void main(String args[]) {
int i=0;
ExecSysCmdThread sysExecCmdThread = null;
while(true){
sysExecCmdThread = ExecSysCmdThread.getInstance();

System.out.println(sysExecCmdThread.executeSystemCommand("CMD /C dir
c:\\windows", null, 5000));
System.out.println("\n\n\nPassCnt: " + i++ + "\n\n\n");
try{Thread.sleep(10);}catch(Exception e){}
}
}

}

 

[Chris Smith]

I modified this class on executing system commands from a version on
koders.com. Being a less than expert coder I was wondering if I could
get some code inspection/style tips to insure it is coded properly.

Here are a few comments off-hand. I didn't read the entire thing
carefully.

First, your naming could use a lot of work. Unless there are reasons
for some of the naming stuff you're doing (for example, company style
guidelines, etc.) you should really strive for less cluttered naming.
There's something to be said for the ability to read code aloud and
understand what's going on. A few specific examples of this:

The use of the m_ prefix for instance fields is rare in Java, in my
experience. Microsoft promoted it primarily with MFC. If you feel the
need to use some kind of prefix on instance variables, then just a
single underscore is much less visually bulky. Better yet, strive for
code that's well-factored enough that you don't need naming help to
distinguish local variables from instance variables.

Why is this called ExecSysCmdThread2? Is there an ExecSysCmdThread1 in
your project? If not, then the number is really confusing and
unnecessary to someone who isn't aware of your thought processes when
you wrote this.

Second, there are a number of places in the code where you catch
Exception and do nothing. Don't ever do that. If there's a specific
exception that you need to ignore, then catch that specific exception,
and preferably include a comment that explains why it shouldn't be
logged/reported/etc. You're inviting long debugging sessions by eating
unexpected exceptions outright.

Third, I'd question the design and division of tasks a little.
Specifically, you have a constructor that initializes half the fields,
and then a method that initializes the other half on the fly before
continuing. That's a bit confusing, and likely to cause problems as the
code is adapted in the future. Can you find stronger abstractions to
divide the code?

A few domain-specific comments, as well:

You have a comment advising people to quote path names with contain
spaces. Unfortunately, this won't work. The version of Runtime.exec
you use just tokenizes by spaces, and doesn't implement any
sophisticated shell-like parsing. For that reason, you should really
provide for the use of the String[] version of exec, which allows you
more control over the division of the command into separate arguments.

Finally, this has been hashed out quite a bit, and it's generally the
consensus that it's cleaner to implement the Runnable interface than to
extend the Thread class. You then pass your Runnable implementation to
an object of the generic Thread class. If you think about it, you
aren't defining a new kind of thread; instead, you're just defining
something for a thread to do; so it doesn't make sense to subclass
Thread.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Andrew Thompson]

..style tips... ....
private String m_InetAddrName = null;

<tip>
...don't post tabs to usenet.

People often like to see the basic effect of code before
they look closely at it. If posting tabs causes line-wrap
that breaks the code from copy/paste/compile first go,
people are less likely to look at it further.
</tip>

The first compile was 33 errors..