Code Review Request

Discussion in 'Java' started by pitoniakm@msn.com, Jun 30, 2005.

  1. Guest

    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 ().
    *
    * 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.io_OutputStream;
    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){}
    }
    }

    }
    , Jun 30, 2005
    #1
    1. Advertising

  2. Chris Smith Guest

    <> wrote:
    > 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
    Chris Smith, Jun 30, 2005
    #2
    1. Advertising

  3. On 30 Jun 2005 05:35:08 -0700, wrote:

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

    --
    Andrew Thompson
    http://www.PhySci.org/codes/ Web & IT Help
    http://www.PhySci.org/ Open-source software suite
    http://www.1point1C.org/ Science & Technology
    http://www.LensEscapes.com/ Images that escape the mundane
    Andrew Thompson, Jun 30, 2005
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. P Kenter

    Request for code review

    P Kenter, May 28, 2004, in forum: C++
    Replies:
    4
    Views:
    354
    P Kenter
    Jun 2, 2004
  2. Ben Hanson

    Request for Code Review

    Ben Hanson, Jul 2, 2004, in forum: C++
    Replies:
    19
    Views:
    654
    Chris Gordon-Smith
    Jul 4, 2004
  3. Debashish Chakravarty

    request for code review

    Debashish Chakravarty, Nov 18, 2003, in forum: C Programming
    Replies:
    3
    Views:
    381
    Robert Stankowic
    Nov 18, 2003
  4. Adam Monsen

    code review request: basic file I/O

    Adam Monsen, Aug 16, 2004, in forum: C Programming
    Replies:
    9
    Views:
    329
    Arthur J. O'Dwyer
    Aug 19, 2004
  5. www
    Replies:
    51
    Views:
    1,459
Loading...

Share This Page