NX: Exception handling implementing the DBAccess

[Jim Yingst]

[Max]: For example, the ArrayList.add() method returns a boolean that returns true if the add was successful. If you don't check that result, then the ArrayList.add() might quietly fail.
Mmm, bad example. ArrayList is defined to always return true for this method - if you know you're using an ArrayList, or any List, there's no way it can fail. (Some Lists might throw UnsupportedOperationException instead, but that's different.) If you're dealing with certain other collection types such as HashSet, add() may indeed fail. So if we replace ArrayList with HashSet, the example is valid.

[Jim Yingst]

[Vlad]: Just one comment: in my assignement there null is definf for findbyCretireia for the case where nothing was found (So I will never get NullPointerException). A user will think there no records in the database, but actually there is something wroth with database...
So, if no records are found, you're supposed to return null? That's unfortunate, as returning a zero-size array seems to make more sense and will require less special handling later. And because this means that we can't use null as an error indicator, if null simply signals "no records". But if that's what your assignment says, then I guess that's what you have to do. I know that some other assignments (mine, for example) do not have this requirement, so returning null would be an acceptable error signal.
I suppose you could use a zero-length array to signal an error, since that option is evidently unused by your API. The disadvantage here is that this is extremely counterintuitive - it becomes the opposite of what Max originally suggested, which was zero-length for no records found, and null for error.
What about returning one or more negative numbers in the array? This has the advantage that you can return valid results and error indicators in the same array. If you found 99 good records and one bad one that threw an error, you can return the row numbers of the 99 good records, and one -1 indicating there was a problem. The client application can decide how/if to present this info. The main disadvantages here are (1) the API says you're supposed to return an array of record numbers; negative numbers aren't record numbers, so we're at least bending the API here. This is risky, and at the very least will require careful documentation. And (2) you still can't contain as much info in these negative numbers as you can in an Exception. (Returning null for error has the same issue.) This isn't necessarily a problem though, as long as the server logs the additional info - the client just needs an indication that something weird happened.
And now going back a bit - I believe this was about the create() method, though much of the discussion can apply elsewhere:
[Max]:Let's take a look at our options here
#1 Swallow and Log the IO exception, and return a negative number. Document that this means that something went wrong @ the IO level.
POSITIVE: it adheres to the specification of your client. The client(Sun) has told you exactly the sorts of technical problems they want to know about, namely the DupliateKeyException, and that's exactly what you're doing.
NEGATIVE : In our opinion, the client should be notified of IO problems explicitly by throwing an IOException
Also, returning a negative number may be against the API we're required to implement. I'd look carefully at the phrasing for your assignment here. But this is still not a bad option if the API doesn't contradict it directly.
Tony's requirement is just like my own in this respect - it says "returns the record number of the new record." Well, a negative number isn't a record number, so that's a cause for concern at least. However the phrasing presupposes that there is a new record to return. Normally I'd greatly prefer to throw an exception if there's no record, but since the only checked exception we're allowed is DuplicateKeyException, and that's even more semantically incorrect than a negative record number, I'd prefer the negative number. At least the negative number cannot be misinterpreted. It's clearly not a record number, therefore something is wrong. DuplicateKeyException could well result in the user seeing a message about duplicate keys, which is completely wrong. I'd rather have "something is wrong" with no further explanation, instead of giving misleading info.
So, given the inherent limitations of the API here, returning -1 seems acceptable to me. But it is at least bending the API, so each programmer should consider if they think it's acceptable under the circumstances.
#4 Throw an unchecked exception
POSITIVE: tells the client exactly what went wrong, and doesn't technically violate the specification
NEGATIVE: violates the client's requirements De Facto, and causes a crash, because the client, by definition, cannot be prepared for an Unchecked Exception.
Ummm, that's a bit of an overstatement. If the client knows only about the DB interface (or DBAccess or whatever it's called) then they can't know about that unchecked method specifically. But they can certainly could be prepared to catch a generic Exception (or RuntimeException) and handle it somehow. Moreover, if the client is aware of our specific implementation of the Data class, and has access to the documentation for it, they could certainly see documentation for a specific subclass of RuntimeException that we choose to throw. We have control of our own client code after all, so we can certainly handle the exception properly in the code we turn in. The possible problems are with (a) junior programmers who don't read our docs, and (b) other programs developed specifically for the DB interface, with no knowledge of our specific application. Which may or may not exist; but it would be nice to plan for them if they do. To be fair though, those groups are going to have some problems with the negative number solution as well.
I'm not really happy with this solution (throwing an unchecked exception), and we lose the benefit of compiler-enforced exception handling. But I'm not really happy with any of these solutions either, and this one isn't nearly as bad in comparision as Max makes it out to be, IMO. It was my #1 choice until I considered the negative number option; now I think it's #2.

[Max Habibi]

Originally posted by Jim Yingst:
[Max]: For example, the ArrayList.add() method returns a boolean that returns true if the add was successful. If you don't check that result, then the ArrayList.add() might quietly fail.
Mmm, bad example. ArrayList is defined to always return true for this method - if you know you're using an ArrayList, or any List, there's no way it can fail. (Some Lists might throw UnsupportedOperationException instead, but that's different.) If you're dealing with certain other collection types such as HashSet, add() may indeed fail. So if we replace ArrayList with HashSet, the example is valid.

True, it is a bad example. I should probably have used String.indexOf
M

[Max Habibi]

Originally posted by Jim Yingst:
[Max]:Let's take a look at our options here
#1 Swallow and Log the IO exception, and return a negative number. Document that this means that something went wrong @ the IO level.
POSITIVE: it adheres to the specification of your client. The client(Sun) has told you exactly the sorts of technical problems they want to know about, namely the DupliateKeyException, and that's exactly what you're doing.
NEGATIVE : In our opinion, the client should be notified of IO problems explicitly by throwing an IOException
Also, returning a negative number may be against the API we're required to implement.
I'd look carefully at the phrasing for your assignment here. But this is still not a bad option if the API doesn't contradict it directly.
Tony's requirement is just like my own in this respect - it says "returns the record number of the new record." Well, a negative number isn't a record number, so that's a cause for concern at least.
<snip>
So, given the inherent limitations of the API here, returning -1 seems acceptable to me. But it is at least bending the API, so each programmer should consider if they think it's acceptable under the circumstances.

I don't think it is: further, is very defensible approach, given way that, say, String.indexOf works. It's an intuitive approach, if you adhere to the sense of the language itself.

#4 Throw an unchecked exception
POSITIVE: tells the client exactly what went wrong, and doesn't technically violate the specification
NEGATIVE: violates the client's requirements De Facto, and causes a crash, because the client, by definition, cannot be prepared for an Unchecked Exception.
Ummm, that's a bit of an overstatement.

Not at all. Since it's not reasonable to assume that clients will catch runtime exceptions when calling your method, then you are causing a de facto crash. And as demanding that middle tier programmers read the doco to 'know' that you're throwing unchecked exceptions: hooey. Why not take the cleaner route and ask them to read to documentation so that they check the return value, or investigate the Exception? Either of these options are better then circumspection the spec by throwing runtime exceptions are really a means of obscuring checked exceptions.

I'm not really happy with this solution (throwing an unchecked exception), and we lose the benefit of compiler-enforced exception handling. But I'm not really happy with any of these solutions either, and this one isn't nearly as bad in comparison as Max makes it out to be, IMO. It was my #1 choice until I considered the negative number option; now I think it's #2

I don't think that wrapped in checked exceptions in unchecked one in order to meet the letter of a specification( not, IMO, it's intent) is the innocuous compromise that Jim makes it out to be. It's seems less like bending the rule more like torturing it into silence and submission . However, that's just IMO, YMMV.
All best,
M

[Wagner Danda Da Silva Filho]

Originally posted by Tony Collins:
Server
Required Interface
Your data access class must be called "Data.java", must be in a package called "suncertify.db", and must implement the following interface:

 
package suncertify.db;
public interface DBAccess
{
// Reads a record from the file. Returns an array where each
// element is a record value.
public String [] readRecord(long recNo)
throws RecordNotFoundException;
// Modifies the fields of a record. The new value for field n 
// appears in data[n]. Throws SecurityException
// if the record is locked with a cookie other than lockCookie.
public void updateRecord(long recNo, String[] data, long lockCookie)
throws RecordNotFoundException, SecurityException;
// Deletes a record, making the record number and associated disk
// storage available for reuse. 
// Throws SecurityException if the record is locked with a cookie
// other than lockCookie.
public void deleteRecord(long recNo, long lockCookie)
throws RecordNotFoundException, SecurityException;
// Returns an array of record numbers that match the specified
// criteria. Field n in the database file is described by
// criteria[n]. A null value in criteria[n] matches any field
// value. A non-null  value in criteria[n] matches any field
// value that begins with criteria[n]. (For example, "Fred"
// matches "Fred" or "Freddy".)
public long[] findByCriteria(String[] criteria);
// Creates a new record in the database (possibly reusing a
// deleted entry). Inserts the given data, and returns the record
// number of the new record.
public long createRecord(String [] data)
throws DuplicateKeyException;
// Locks a record so that it can only be updated or deleted by this client.
// Returned value is a cookie that must be used when the record is unlocked,
// updated, or deleted. If the specified record is already locked by a different
// client, the current thread gives up the CPU and consumes no CPU cycles until
// the record is unlocked.
public long lockRecord(long recNo)
throws RecordNotFoundException;
// Releases the lock on a record. Cookie must be the cookie
// returned when the record was locked; otherwise throws SecurityException.
public void unlock(long recNo, long cookie)
throws SecurityException;
}

Any unimplemented exceptions in this interface must all be created as member classes of the suncertify.db package. Each must have a zero argument constructor and a second constructor that takes a String that serves as the exception's description.
Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.
Network Approaches
Your choice of RMI or serialized objects will not affect your grade, but no other approach is acceptable. In either case, the program must allow the user to specify the location of the database, and it must also accept an indication that a local database is to be used, in which case, the networking must be bypassed entirely. No authentication is required for database access.
Locking
Your server must be capable of handling multiple concurrent requests, and as part of this capability, must provide locking functionality as specified in the interface provided above. You may assume that at any moment, at most one program is accessing the database file; therefore your locking system only needs to be concerned with multiple concurrent clients of your server. Any attempt to lock a resource that is already locked should cause the current thread to give up the CPU, consuming no CPU cycles until the desired resource becomes available.

I understand that the DBAccess interface and Data class must be used in the server side to read the database file, but, this is not a rule for the client side. So, why not use a Wrapper that implements the same methods os DBAccess and also throws the apropriated exceptions?
See (and comment, please) this example:

// The class that really reads the database file (used only by the server)
public class Data implements DBAccess {
...
    protected IOException ex;
    public String [] readRecord(long recNo) throws RecordNotFoundException {
         try {
            // Try to read the data
         } catch(IOException ioEx) {
            this.ex = ioEx;
            return null;
         }
    }
    public IOException getIOException() {
         IOException ioEx = this.ex;
         this.ex = null;
         return ioEx;
    }
...
}
// The class used by the client side (user interface) 
public class WrapperData {
...
    // The class thats is being wrapped
    Data data;
    public String [] readRecord(long recNo) throws RecordNotFoundException, IOException {
        // Read the record
        this.data.readRecord(recNo);
        // Check (and throw) if an error occurred
        IOException ioEx = this.data.getIOException();
        if( ioEx != null ) {
            throw ioEx;
        }
    }
....
}

Best regars,
Wagner da Silva

[Jim Yingst]

[Jim]: So, given the inherent limitations of the API here, returning -1 seems acceptable to me. But it is at least bending the API, so each programmer should consider if they think it's acceptable under the circumstances.
[Max]: I don't think it is: further, is very defensible approach, given way that, say, String.indexOf works. It's an intuitive approach, if you adhere to the sense of the language itself.
We're not far apart here; I agree it's defensible. But I think there's enough rule-bending here to at least mention it. Looking at String's indexOf() for comparison - the documentation there says:
Returns: the index of the first occurrence of the character in the character sequence represented by this object, or -1 if the character does not occur
Here the -1 option is clearly stated in the docs; moreover the "or" indicates they're acknowledging that -1 is not the index of anything; it's something else entirely. In comparison, our create() method just says it "returns the record number of the new record". To me, that implies that I can assume that what's returned is indeed a record number of a valid record which has in fact been created. Unless I read otherwise in supplementary docs somewhere else. Or unless I actually observe the -1 being thrown; I could probably figure out what it meant.
[Max]: #4 Throw an unchecked exception
POSITIVE: tells the client exactly what went wrong, and doesn't technically violate the specification
NEGATIVE: violates the client's requirements De Facto, and causes a crash, because the client, by definition, cannot be prepared for an Unchecked Exception.
[Jim]: Ummm, that's a bit of an overstatement.
[Max]: Not at all.
Well, "the client, bu definition, cannot be prepared for an Unchecked Exception" is definitely an overstatement. I can write a client that is prepared for this; it's not hard. If you meant "the client probably will not be prepared for an unchecked exception" or "we can't ensure that the client will be prepared for an unchecked exception" then OK, I agree. Unless extra steps are taken to make sure client coders are aware of the unchecked exceptions - and we don't get the compiler's help in enforcement, which is a big loss.
[b][Max]: Since it's not reasonable to assume that clients will catch runtime exceptions when calling your method, then you are causing a de facto crash.
I don't fully concede the "not reasonable to assume". This is a significant risk, true. But if I write the client code, I can guarantee that the clients will catch the exception. We have no real indication that there's any other client code out there today. Though it's possible there is, and B&S neglected to mention it. I think the main possible additional client code is in the future; hopefully they'll have access to our improved documentation, and/or have our own code to look at to see how to handle
And as demanding that middle tier programmers read the doco to 'know' that you're throwing unchecked exceptions: hooey. Why not take the cleaner route and ask them to read to documentation so that they check the return value, or investigate the Exception?
Well, all these options really require that the other programmers read our documentation in order to make sense of the results. If they've read our docs, then handling the results is pretty straightforward, whether it's catching an unchecked exception, doing getCause() on a checked exception, or
checking if recNo < 0. If they've read our docs, none of these is a problem really. If not, if they've only read the DB interface, then all these options can cause problems. The question then becomes: which of these options will do the least damage if client code is not expecting it? Let's consider:
Returning -1: if the client doesn't know to check for recNo < 0, what happens? Well, if we're lucky they at least save the recNo and maybe try to do something else with it later, like read() or update(). At which point they get RecordNotFoundException, which may be a bit confusing ("I just created it. What happened to it?") but at least alerts them that something bad has happened, and their program is still running. If we're less lucky though, the client may not do anything with the return value. Maybe the client program is for data entry, and there's a user addig new records. She adds one, the system looks OK, she adds another, looks OK, she adds a hundread more. Then eventually she does a find() to see all the records she added, and nothing's there. In this case at least, I think the user would have preferred to just have the program crash when the first record was added. Then she wouldn't have wasted all that effort on the subsequent rows - she'd have e-mailed the server admin to get the server's problem sorted out before continuing.
Throwing SomeNewRuntimeException: most likely, if the client hasn't read our docs, this will lead to the program exiting, or maybe the GUI event handler hangs and the user is looking at a locked-up GUI. Which is generally viewed as bad. But at least they know an error occurred, and didn't waste further effort. They may restart without too much difficulty, and either the error recurs or it doesn't, and the user can proceed accordingly. The main downsides are (1) the crash looks icky to the user (which is really the client programmer's fault; I can live with that), and (2) the user might lose some unsaved data during the crash. The latter isn't an issue for our current application, but could be for some future enhnacements. With luck the unsaved data is still avilable on a frozen-up GUI. If not, well that's bad. But I don't see it as any worse than the situation for the data entry worker in the previous paragraph. Either way, work may be wasted.
Throwing a DuplicateKeyException with nested IOException: acutally I think this may give the best worse-case behavior. The exception should be caught and handled, and unless the client programmer is completely incompenet (e.g. he's the sort of person who creates empty catch blocks without good reason) the user should see a message that a problem has occurred. The message way well be very confusing and misleading, but at least they know something happened. Worst-case here seems to be: user becomes increasingly frustrated at nonsensical program, complains to client programmer, who is perplexed until he eventually figures out what's going on, at which point he complains about the shoddy way our program was written, throwing a nonsensical DuplicateKeyException. Now if we're around to defend ourselves we can say hey, look, here's the documentation, didn't you read it? (Or didn't your boss pass on the documentation we provided?) If we're not around, we probably just get blamed for writing a crappy program. Well, that probably would happen under any of the previous options too, if they didn't read the docs and we're not around to defend ourselves. Though I think the first two options are much clearer to debug - the DuplicateKeyException is misleading, which I often may consider a greater evil than just plain crashing. (This is true when you're debugging at least, though end users may have a different perspective.)
Either of these options are better then circumspection the spec by throwing runtime exceptions are really a means of obscuring checked exceptions.
The spec was evidently written without consideration of "what happens if something goes wrong" other than the specific causes they cited (invalid recNo or "duplicate key" which they never bothered to explain anyway. I don't see this as circumventing anything so much as addressing something they didn't consider. "Obscuring checked exceptions" - eh, all a matter of perspective; if something does go wrong throwing a RuntimeException here will have the highest probability of generating a really obvious error message detailing just what and where the problem really is. Giving up compile-time enforcement of exception handling is a problem, but not the end of the world, IMO.
[Max]: I don't think that wrapped in checked exceptions in unchecked one in order to meet the letter of a specification( not, IMO, it's intent) is the innocuous compromise that Jim makes it out to be. It's seems less like bending the rule more like torturing it into silence and submission . However, that's just IMO, YMMV.
Caveat emptor, cash only, all sales are final, etc.
Well, returning an undocumented -1 or returning a misleading exception aren't always so innocuous either, IMO.
But I don't think we have a great fundamental difference here (unlike a certain other topic); we just assign different weights to the various pluses and minuses.
Cheers...

[Jim Yingst]

[Wagner]: I understand that the DBAccess interface and Data class must be used in the server side to read the database file, but, this is not a rule for the client side. So, why not use a Wrapper that implements the same methods os DBAccess and also throws the apropriated exceptions?
Well, something similar to that might be possible. However remember the that system as a whole needs to be able to serve multiple clients concurrently. Many people in this forum are doing that with a Sinlgleton or singleton-like design, where all clients access the same Data instance. If you do that here, then how does each client know which client generated an exception? They can call getIOexception() to find out if an exception occured, but they don't know who caused it.
On the other hand, if each client has a separate Data instance, well, that can work. It's a little convoluted to catch the original IOException, store it, later rethrow it, and then someone still has to catch it and decide what to really do with it. But this approach can workI suppose...
However, what if some other program is written to use the DB inteface, and they try to use your Data implementation, with no wrapper? That is, we know that we can write our own code to handle these exceptional conditions appropriately, using any of the several strategies outlined so far. The dander is, what if someone makes a future enhancement without fully understanding our design? They may not use the WrapperData class at all, just the Data class. How will they know they should check getIOException()? Well, the same way they'd know to check for -1, or catch SomeUncheckedException, or catch DuplicateKeyException and use getCause() to find out what really happened. They need to read our documentation for the Data class, or study our code enough to understand how it really works. If they do that, great - any of the other methods will work too. If they don't - well, that's the problem.
Personally I think that the getIOException() is unnecessary. The client doesn't really care what the problem is on the server; they just need to know that a problem occurred. They get that from checking if the return value is null, or -1, or if some exception has occurred. They don't really need any more info. The server log files should have the extra info; the client doesn't really need it.
However, I do think you can probably make a perfectly good program using this technique. It's just a little more complex and counterintuitive, to me, than I feel is necessary. But if you like it, go ahead and do it.

[S Bala]

At least we have an option of returning something in case of find method.
But, look at the update method.

// Modifies the fields of a record. The new value for field n 
// appears in data[n]. Throws SecurityException
// if the record is locked with a cookie other than lockCookie.
public void update(int recNo, String[] data, long lockCookie)
throws RecordNotFoundException, SecurityException;

Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.

How do I throw an exception (eg. InvalidRecordException) if the record length being updated is not correct - Client gave more characters than required. Also, there is UnsupportedEncodingException. Can we classify both the above scenarios under RecordNotFoundException. But, the "quote" says throw them only if deleted or not found.
SB

[S Bala]

I was reading a web posting on exception tunneling. Sounds interesting.
Define a private exception which is subclass of Runtime exception.
Throw our unrecoverable exceptions like IOException in the read method as a Runtime exception defined by us. Use a facade, our remote class to catch the specific runtime exception and throw a checked exception to the user.
At least this will avoid catching the same exception in every layer.
Sb

[Vlad Rabkin]

Hi,
It is exactly what I will probably do.
In this Runtime exepction is catched on the server.
There is a class EJBException in J2EE world, which is runtime exception,
but a client receives RemoteException (The EJB container parses the exception), it is done because in J2EE it is not recommended to throw a RemoteException by your own.
So, it sounds good to me.
Vlad

[Tankred Smult]

I'm doing the Contractors assignment.
The problem is; we all want to throw IOExceptions, but we are not allowed to by the DBMain interface. Whrapping the IOException in one of the allowable checked exceptions is counterintuitive for client programmers. Unchecked exceptions are bad because it doesn't force clients to catch the exceptions.
Some solutions have been descibed in this thread, but I find them all a bit hacky. Then again, because of the DBMain interface there is no elegant solution to the problem.
However, the best solution I see is to throw a custom unchecked exception called UncheckedIOException from all the methods in Data where IOExceptions occur, using the actual IOException as the cause. Then document this in the javadoc like there's no tomorrow. And do not make Data public, instead make it only package visible!
Then make a new class called IOData, which have the exact same methods as data, but it will also declare IOExceptions. This class works as a adapter for the dataclass. It just calls the corresponding methods in the data class, catches the UncheckedIOExceptions, extracts the IOException whrapped inside and rethrows that IOException. The IOData class is the one available publicly (outside the package).
This way, at least you encapsulate the strange constrains gived by the DBMain interface within the suncertify.db package, hiding this awkwardness from clients(classes outside the db-package).

[Jim Yingst]

[spec]: Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.
[S Bala]: How do I throw an exception (eg. InvalidRecordException) if the record length being updated is not correct - Client gave more characters than required.
An IllegalArgumentException is perfectly appropriate here. The client's supposed to check the length in advance. We don't need a checked exception for this.
Also, there is UnsupportedEncodingException. Can we classify both the above scenarios under RecordNotFoundException.
I wouldn't. If you use NIO's Charset class for encoding and decoding, it throws UnsupportedCharsetException rather than UnsupportedEncodingException. UCE is a RuntimeException, while UEE is not. I think a RuntimeException is fine here, especially since the Java platform is guaranteed to support US-ASCII - there is no real need for any exception here. Sure, the encode/decode methods declare them, but so what - if you use US-ASCII, you will not get a Unsupported[Whatever]Exception, so it really doesn't matter.
But, the "quote" says throw them only if deleted or not found.
The quoted spec did not say "only" - you inserted that. Now personally I don't really like using RecordNotFoundException for any reason other than the ones they explicitly stated - but nothing in the instructions forbids it, either.

[S Bala]

Jim:
The quoted spec did not say "only" - you inserted that.

- Agreed
But, the spec reads reads close to that.
- SB
[ August 12, 2003: Message edited by: S Bala ]

[Dana Hanna]

To add my two cents, for what it's worth, I say throw a child of RuntimeException.
The exception is not expected, and if it happens something is seriously wrong. Recovering from it is outside of the specification.