TRUNK-4696: added specific exception classes for the Order Service #1860
Conversation
|
@samshuster, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bmamlin, @wluyima and @teleivo to be potential reviewers. |
| * The specific error classes define only a default constructor if they are meant to be used with only a single error message. | ||
| * In other cases, there should be static factory methods that can be used to create the error class with the specific error message needed. | ||
| */ | ||
| public class OrderExceptions { |
There was a problem hiding this comment.
@samshuster do you mind throwing some more light as to why you prefer creating a new OrderExceptions class for grouping purposes instead of just putting them in the org.openmrs.api or any other package, as the rest of the existing custom exception classes? :)
There was a problem hiding this comment.
will obviously defer to you on whats best.
But here is why I made the decision not to make them there own classes.
- There is a lot of exceptions being created and it would pollute the number of classes in that package.
- These exceptions are all similar to each other and it might make it easier on user to see a list of all different Order Exception classes.
- Unless static import used, when throwing them, the name is very expressive. new OrderExceptions.CannotEditExistingOrderException()
Downsides:
- Not consistent with rest of project
- OrderExceptions is essentially a package name. Making a separate package called orderexceptions would help alleviate concern 1 and 2.
There was a problem hiding this comment.
i will move them to custom exception package
| public static final long serialVersionUID = 22121215L; | ||
|
|
||
| public CannotDiscontinueOrderWithActionException() { | ||
| super("Order.action.cannot.discontinued", new Object[] { DISCONTINUE }, null); |
There was a problem hiding this comment.
If just passing null for the third argument, why not just use the two argument constructor? :)
samshuster
force-pushed
the
TRUNK-4696
branch
from
f7b79fd
to
977cf97
Compare
samshuster
force-pushed
the
TRUNK-4696
branch
from
977cf97
to
991a898
Compare
Modifying some Order Exception definitions to be more in line with the messages.properties. Fixing test to map to these new definitions. moving exceptions to separate classes. Changing call to use overloaded method without throwable adding license to files
samshuster
force-pushed
the
TRUNK-4696
branch
2 times, most recently
from
623ffe0
to
b33cae6
Compare
Description
Modifying some Order Exception definitions to be more in line with the messages.properties. Fixing test to map to these new definitions.
Related Issue
see https://issues.openmrs.org/browse/TRUNK-
Checklist:
git pull --rebase upstream master.mvn clean packageright before creating this pull request andadded all formatting changes to my commit.