Enhancement/refactor exceptions [FIXES #32] #33
Conversation
|
Hello @osopromadze, thank you very much for this PR! It looks great at a first glance. We will review it in the next few days and give an answer at the beginning of next week. |
|
Great @chaoran-chen, thanks for letting me know 👍 |
…ement/refactor_exceptions
|
Hi. Would it be possible to remove projectlombok from the dependencies since we are also not using it in the other classes? But the library is very nice and maybe we will introduce it consistently for the whole project at a later point in time. |
There was a problem hiding this comment.
Hi @osopromadze, thank you for your PR. It looks good. I just added a few comments.
| @@ -132,7 +137,7 @@ public int createAccount(RegistrationRequest registration) throws AccountCreatio | |||
| */ | |||
| public String login(Account account) { | |||
| if (account.getId() != null) { | |||
| throw new IllegalArgumentException("The account id is not null."); | |||
| throw new AccountCreationException().withErrorCode(ErrorCodeEnum.ERR_ACC_002); | |||
There was a problem hiding this comment.
This has nothing to do with account creation. So maybe better use a IllegalArgumentException or IllegalRequestParameterException.
| DeviceAvailabilityCreationException.class, | ||
| IllegalRequestParameterException.class | ||
| }) | ||
| public ResponseEntity<Object> handleAccountCreationException(HttpServletRequest request, BaseException exception, Locale locale) { |
There was a problem hiding this comment.
Because you are handling different exceptions here it would be better to choose a more general method name.
There was a problem hiding this comment.
@dajenet good idea choosing more general name, or handle different type of exceptions in different methods, but as method body is same thats why I put them in same. Feel free to change it as you consider.
Yes, no problem, Just added project lombok to easily generate constructros, or getters and setters. Can be removed easily and write code without it. |
There was a problem hiding this comment.
The import in src/test/java/science/icebreaker/device_availability/DeviceAvailabilityServiceTest.java#20 needs to be adapted since the DeviceAvailabilityCreationException was moved. And for consistency, DeviceAvailabilityNotFoundException should also be moved to the science.icebreaker.exception package.
@chaoran-chen done. All tests passed and moved |
fixes #32
Moved all exceptions into the same package. They are extended from BaseException class. Created messages.properties in resources/i18n folder, and exceptions get error messages from that file. Created global exception handler class and there is constructing final output error object with status code, timestamp and message.