why does OrderService.discontinueOrder throws Exception

Development
1

Hello :slight_smile:

can someone please tell me why

OrderService.discontinueOrder(Order orderToDiscontinue, Concept reasonCoded, Date discontinueDate, Provider orderer,
	        Encounter encounter) throws Exception
OrderService.discontinueOrder(Order orderToDiscontinue, String reasonNonCoded, Date discontinueDate, Provider orderer,
	        Encounter encounter) throws Exception

throw Exception ?

I see that the OrderServiceImpl wouldnt need to, if it wasnt for the interface.

Am interested into why this is and what a dev should do with it when using these methods?

Most methods in the openmrs API throw APIException which can be caught and dealt with but why choosing a so general Exception :slight_smile:

Thanks!!

2

Probably a bug, throwing Exception is rarely justified.

3

I agree with @lluismf :slight_smile:

4

Ahh, we just released openmrs 2.0.0

I guess this can only be removed in a major release since its part of the method signature.

Should an issue to remove this be created?

5

I believe that if someone is catching it in a controller and you remove it from the signature nothing will happen (it will still compile). Because Exception includes both checked and runtime.

6

thanks @lluismf!

@darius what do you think, should we remove throws Exception from OrderService.discontinueOrder

As a side note: I see that the checkstyle doesnt prevent this type of throws

should this checkstyle config also be adjusted.

  • I think currently checkstyle is not configured to break the build on Travis CI (it doesnt run on PRs), should it?
7

Sonar is already detecting this problem (with severity = Major)

2016-08-01 13_08_54-OpenMRS.png1127Ă—454 36.2 KB

https://ci.openmrs.org/sonar/drilldown/issues/1865?&rule=squid%3AS00112&rule_sev=MAJOR&severity=MAJOR

1 Like
8

am I now allowed to call myself “analog sonar” :yum:

9

I do agree that Exception is too generic, I believe the undocumented convention is to add throws APIException to service methods and DAOException to DAO methods. Since these are unchecked exceptions this leaves the API backwards compatible even when new exceptions are thrown from service methods as long as they are of the same type or a sub type. But of course another person might urgue that we can then get rid of the throws clause since they are unchecked exceptions.

10

I do :slight_smile: it’s just noise

11

+1 to just switching it to throws APIException in the next maintenance release.

I agree with Lluis that this just adds noise, but I wouldn’t bother changing that in just one method, if it’s the convention everywhere else.

-Darius (by phone)

12

created ticket https://issues.openmrs.org/browse/TRUNK-4909 with Fix Version/s: Platform 2.0.1

13

Thanks Televio

1 Like