Implement daily worked time rounding #248
Conversation
There was a problem hiding this comment.
Hi @sant0s12,
thanks for your pull request! I'm sorry it took me so long to review it - I just started into a new job which took some adjusting, and then I was away on family vacation. But now I provided some feedback (see code comments).
For your feature to be complete, it will be necessary to also include rounding for the reports if it's activated. The users won't tolerate different numbers in the app and in the reports (there already were some issues reported regarding this in the past).
But more importantly the rounding formula seems to cover only some cases. As I wrote in the code comment I'd like to have a unit test for this calculation. If you are unsure how to approach this, I can also help out with that, but at the moment the rounding seems not to work correctly. So if you don't add a unit test, please at least fix the method roundUpToMultiple().
Thanks again for your contribution! It didn't deserve to be ignored for so long!
| if (handleFlexiTime) { | ||
| this.flexiReset = timerManager.getFlexiReset(); | ||
| } else { | ||
| this.flexiReset = FlexiReset.NONE; | ||
| } | ||
|
|
||
| this.roundingEnabled = preferences.getBoolean(Key.ROUNDING_ENABLED.getName(), false); | ||
| if (preferences.getBoolean(Key.ROUNDING_ENABLED.getName(), false)) { |
There was a problem hiding this comment.
This can be
if (roundingEnabled) {
instead (we just evaluated the same statement used here).
| if (handleFlexiTime) { | ||
| this.flexiReset = timerManager.getFlexiReset(); | ||
| } else { | ||
| this.flexiReset = FlexiReset.NONE; | ||
| } | ||
|
|
||
| this.roundingEnabled = preferences.getBoolean(Key.ROUNDING_ENABLED.getName(), false); |
There was a problem hiding this comment.
this. is superfluous here (above in this.flexiReset it's also superfluous).
| if (multiple == 1) { | ||
| return value; | ||
| } else { | ||
| return (value + multiple - 1) / multiple * multiple; |
There was a problem hiding this comment.
This won't work in all cases. For the inputs value = 43, multiple = 15 it should round to 45. But it doesn't, the method would return 57 with these inputs.
To be on the safe side, I'd recommend creating a unit test for the calulation method where some "normal" cases and many (if not all) of the edge cases are checked. For a start you can copy TimeCalculatorTest to TimeCalculatorV2Test and adjust the content accordingly.
There was a problem hiding this comment.
Possible values of multiple are: 1, 2, 3, 4, 5, 6, 10, 12, 15, 20, 30, 60
But probably only the values 5, 10, 15, 30 and 60 will be used in real life...
There was a problem hiding this comment.
Oh, that's weird. I'll write some tests for it.
Also, I do use 6 as a multiple myself since my work tracks time to an accuracy of 0.1 hours. Therefore I think it makes sense to leave it up to the user to choose any divisor of 60 :)
| @@ -361,7 +374,7 @@ public void calculateNextDay() { | |||
|
|
|||
| currentDayHasEvents = (events != null && !events.isEmpty()); | |||
|
|
|||
| workedTime += calculateWorkTime(events); | |||
| workedTime += roundUpToMultiple(calculateWorkTime(events), smallestTimeUnit); | |||
There was a problem hiding this comment.
This should be wrapped in an if block to only execute it when required:
if (roundingEnabled) {
workedTime += roundUpToMultiple(calculateWorkTime(events), smallestTimeUnit);
}
There was a problem hiding this comment.
While glancing over this again, my previous comment doesn't make sense. The calculateWorkTime(events) would also be only executed if my code snippet was used, so it better should be like:
if (roundingEnabled) {
workedTime += roundUpToMultiple(calculateWorkTime(events), smallestTimeUnit);
} else {
workedTime += calculateWorkTime(events);
}
| } else { | ||
| this.smallestTimeUnit = 1; |
There was a problem hiding this comment.
The else part can be left out if the variable is initialized with the value 1 above:
private final int smallestTimeUnit = 1;
There was a problem hiding this comment.
Maybe I am missing something, but I get an error if I do that.
There was a problem hiding this comment.
Sorry, my bad - the final keyword is the problem. With
private int smallestTimeUnit = 1;
it works.
|
Hi @mathisdt, thanks a lot for your feedback. I'll take a look at it and implement your suggestions. |
|
Hi @sant0s12, I'm interested in seeing this feature deployed. |
|
@Josic490 Yes, but I am a bit busy at the moment, don't know when I'll get around to it. Feel free to address @mathisdt feedback if you want! :) |
Attempts to resolve #147.
At the moment it does not work when generating reports.