[Java-player-developers] Need to be propagate exceptions, not hide them.

[Gareth R. <g_j...@ya...>]

Hi,

I notice from reading your code that you do not handle exceptions in a
safe way. Serious errors need to reported to whatever code is using
this client. If execution cannot continue because of an outright
failure, that needs to be "admitted" immediately while the execution
flow is still near to the problem, not glossed over by catching the
exception :-) For instance, in:
java-player/javaclient/PlayerDevice.java revision 1.4, you do this:

    protected synchronized void readHeader() {
        try {
            t_sec    = is.readInt ();
[snip]
        } catch (Exception e) {
            System.err.println ("[PlayerDevice] : Error when reading
header: " + e.toString ());
        }
    }

If any exception occurs then mysterious failures will occur later in
the application with little indication as to what caused them. The
"rules" in Java are:

1. Throw checked exceptions for conditions that the caller could
reasonably be expected to handle e.g. java.io.FileNotFoundException.
Wouldn't it be bad if a method hid this exception and a user only found
out much later that their data hadn't been processed correctly?

2. Throw unchecked exception for programming errors or conditions that
are completely unrecoverable. e.g. A
java.lang.NegativeArrayIndexException is normally end of program - it
is clearly the fault of the programmer.

3. Don't "catch Exception" - it's too broad and you might catch
something that should be propagated back to the caller.


From these rules, I would rewrite this method (and all the others), to
throw a custom exception e.g. PlayerException.

    protected synchronized void readHeader() throws PlayerException {
        try {
            t_sec    = is.readInt ();
            t_usec   = is.readInt ();
            ts_sec   = is.readInt ();
            ts_usec  = is.readInt ();
            reserved = is.readInt ();
            size     = is.readInt ();
        } catch (IOException e)  // Only one that we need to catch
        {
            // Allow the caller to handle this exception, if it can,
and store the cause.
            throw new PlayerException("[PlayerDevice] : Error when
reading header", e);
        }
    }

Personally, from what I've seen so far of the code I'd make the
PlayerException a checked exception, i.e. public class PlayerException
extends Exception. That's because it looks to me like reestablishing a
connection would be a sensible way in which a client could recover
(i.e. there is a way). However, that will change the published API (for
the better). If you instead make it an unchecked exception then it will
extend RuntimeException.


As it currently stands I think that this exception hiding is a
significant limitation of the library, but if you're using Eclipse, as
you've said you are, then I don't think this will take too long to
refactor and will provide real benefits :-)

Yours,

Gareth Randall



		
___________________________________________________________ 
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com