awasum commented on a change in pull request #1022:
URL: https://github.com/apache/fineract/pull/1022#discussion_r439499975
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/adhocquery/service/AdHocScheduledJobRunnerServiceImpl.java
##########
@@ -88,10 +88,8 @@ public void generateClientSchedule() {
next = start.plusDays((int) (long)
adhoc.getReportRunEvery());
run = Days.daysBetween(start, end).getDays()
>= adhoc.getReportRunEvery();
break;
- default:
- throw new IllegalStateException();
}
-
+ throw new IllegalStateException();
Review comment:
@percyashu ..Maybe remove this exception? and put it back in the default
case inside the } on line 91 above?
Also if you want to add a default step, you need to check the
`ReportRunFrequency.fromId(adhoc.getReportRunFrequency()` method from line 70
above and see how many different cases needs to be treated, then try to handle
them all. Do we absolutely need a default case in all implementations of Switch
statement?
As seen here:
https://errorprone.info/bugpattern/UnnecessaryDefaultInEnumSwitch, dont put a
default statement if all the cases have been handled. Maybe skip it? Will that
throw an error?
Or given that ReportRunFrequency is an enum and we/Fineract is already
handling all the 5 cases here, we can just throw a `new
AdHocScheduledJobFrequencyException()` error here as default case with a
message like "Correct Frequency not specified, Check the API docs"...
@vorburger @ptuomola your take?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
