sdl_java_suite
sdl_java_suite copied to clipboard
Libs should prefer specific exceptions to generic exceptions
The current libraries don't follow best practices for handling exceptions. Here is a good guide for Java Exception best practices.
For example, SDL only has defined a single custom exception - SdlException and the exception has a SdlExceptionCause field explaining why the exception was thrown. This is really against Java best practices for Exceptions, as throwing a generic exception is hiding the real reason the exception was thrown and doesn't give developers much information when they get the exception.
The current implementation requires developers to check the cause of the exception and add a lot of code into a single catch block instead of giving them the ability to catch different exceptions in different catch blocks based on different exceptions that could be thrown. This also makes it more difficult to throw specific exceptions because whenever an exception is thrown, the method has to do additional work to set the cause field. If the cause field isn't set correctly, catch blocks won't handle it correctly, which will cause bugs that are extremely difficult to track down.
In another example, the following code (from SubscribeVehicleDataResponse) contains a try/catch block around the code that creates a new object using a hashtable and catches a generic exception. If I trace this call up, I can see that the object just passes the hash table up the hierarchy (RPCStruct) and the superclass just saves a reference to that hashtable.
None of these steps could possibly throw an exception, so I believe these try/catch blocks can be removed since it's unreachable code. This issue is contained in a lot of other RPCs classes.
@SuppressWarnings("unchecked")
public VehicleDataResult getGps() {
Object obj = parameters.get(KEY_GPS);
if (obj instanceof VehicleDataResult) {
return (VehicleDataResult) obj;
} else if (obj instanceof Hashtable) {
try {
return new VehicleDataResult((Hashtable<String, Object>) obj);
} catch (Exception e) {
DebugTool.logError("Failed to parse " + getClass().getSimpleName() + "." + KEY_GPS, e);
}
}
return null;
}
I think this issue requires a lot of thought and discussion as to how the generic SdlException could be split up into more specific exceptions. The code also needs to be thoroughly investigated to remove unreachable catch blocks.