best practice of exceptions in a modules API

Development
,
1

Dear experts :slight_smile: (@dkayiwa, @mogoodrich, @mseaton, @darius, @wyclif )

I wanted to ask what the best practice is regarding exceptions in modules and get some input on my thoughts as well.

I want to clean up the API (all the OpenmrsService’s) of the radiology module in terms of its exceptions:

First the main rules I would apply (ideas are not mine, but from Joshua Bloch exceptional book Effective Java :innocent:):

  1. throw checked exceptions only when a user of the API would be able to recover from it given the exception and its additional information
  2. throw RuntimeException’s for programming errors/breach in the contract given by my API (for example if you want to get a RadiologyOrder by its UUID using RadiologyOrderService.getRadiologyOrderByUuid(String uuid) dont pass in NULL). And in my opinion better to throw a RuntimeException in such a case then to return null, so the consumer of the API quickly identifies where the error came from so he can fix his code.

By looking at openmrs-core, for example https://github.com/openmrs/openmrs-core/blob/2.0.x/api/src/main/java/org/openmrs/api/impl/OrderServiceImpl.java I can see that above points do apply.

Now my questions for checked exceptions in modules:

  1. Should a module create its own general exception class like for example in the reporting module? Lets say mine would be called RadiologyException

  2. And if so should this exception be a sublcass of the openmrs.api.APIException?

  3. Then if an exception is so special and needs some additional info I can create specialized exceptions as subclasses from RadiologyException?

  4. But then in the API I only use RadiologyException as checked exception, so client code is only forced to to handle this type, although my API methods can of course throw on of my specialized exceptions.

2

APIException is a runtime exception (and I like it) so these signature is doubtful:

public synchronized Order saveOrder(Order order, OrderContext orderContext) throws APIException {

I don’t see much gain, unless you add attributes or methods specific to Radiology.

It won’t be forced to catch it if inherits APIException of course.

3

  1. Create a new exception class only when you expect some handling of the code to take a significantly different action, based on the exception type. If this is not the case, you will simply be increasing code to maintain. :slight_smile: Also remember not to fall into the trap of wanting to throw custom exceptions for cases where the standard Java exceptions would be enough.

  2. If you have to, and your exception needs to be checked, then APIException is no longer an option because it is not checked.

  3. Same principle as in 1. And if this additional information can fit in an exception message, all the more reason why you may not need the extra class.

4

@teleivo i also forgot to say that there are more community experts than just the ones you mentioned. You should be able to see that from the very first response you got for this thread. :smile:

5

As Daniel mentioned, from a module I typically only add a new exception type if I need to take specific action when it’s thrown.

6

thanks a lot to all for your infos here! certainly did not mean to say anyone listed is not an expert :wink:

7

Do we have documentation for openmrs specific exceptions.Been searching around but in vain ?

I have so far used (ObjectNotFoundException , NoResultException.) but not found them very useful in my use case as they cannot be type cast to PageableResult (for methods with PageableResult return type)

8

Are you looking for the ones in this package? https://github.com/openmrs/openmrs-core/tree/master/api/src/main/java/org/openmrs/api

9

Thanks let me check.