Re: Code Review Request

<pitoniakm@xxxxxxx> 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
.

Relevant Pages