Fixed score when using skips on non-daily habit#1713
Fixed score when using skips on non-daily habit#1713Maxet1000 wants to merge 5 commits intoiSoron:devfrom
Conversation
hiqua
left a comment
hiqua
left a comment
There was a problem hiding this comment.
No change needed in the numerical habit case?
| * sum of the denominator and the number of skips within the interval. | ||
| */ | ||
| @Synchronized | ||
| fun getNumberOfSkipsByInterval( |
There was a problem hiding this comment.
Not really knowledgeable about this, but if you want to make it recursive, might as well follow https://kotlinlang.org/docs/functions.html#tail-recursive-functions, what do you think?
There was a problem hiding this comment.
This wouldn't work here as the 'tailrec' modifier requires that the function calling itself is the last operation that function performs, which isn't the case here. I'm not really knowledgeable about this either, so I might be missing something.
There was a problem hiding this comment.
Yes but you can rewrite so that's the case, i.e. you can change the function so that it's tail-recursive. I think that'd be preferable here.
| rollingSum -= 1.0 | ||
| val nbOfSkips = | ||
| getNumberOfSkipsByInterval(values, offset, offset + denominator) | ||
| if (offset + denominator + nbOfSkips < values.size) { |
There was a problem hiding this comment.
Create a variable for offset + denominator + nbOfSkips to make it easier to read?
| fun getNumberOfSkipsByInterval( | ||
| values: IntArray, | ||
| firstIndexToCheck: Int, | ||
| lastIndexToCheck: Int |
There was a problem hiding this comment.
This parameter name is misleading, because in practice it's not necessarily the last index checked. Can you try to make it clearer for the reader what this does in which cases?
| * the percentage of completed days. | ||
| * | ||
| * If skips are found in the interval, it is expanded until the interval has the size of the | ||
| * sum of the denominator and the number of skips within the interval. |
There was a problem hiding this comment.
There's no denominator in this function so this doc could be improved to make it understandable on its own, can you try to do that?
|
|
||
| @Test | ||
| fun test_getNumberOfSkipsByInterval_NoSkips() { | ||
| val vars = intArrayOf(-1, -1, -1, -1, 3, 2, 2, -1, 2, 2, -1, 2, 2, -1, 3, 2) |
There was a problem hiding this comment.
Here and below: can you hide the integer behind the enum, i.e. use SKIP etc. instead?
|
@hiqua |
|
Requesting this fix @iSoron please. I want to properly skip a workout day for my 3 days a week workout habit but it broke my streak! |
|
@hiqua I've now made the function tail recursive like requested |
|
Could you also rebase on top of the current dev, to make it possible to run the GH actions? Thanks! |
|
I've rebased it! I'm not very familiar with rebasing, so I hope I did it correctly. Please let me know if I should make any more changes. |
|
I've updated it (again) and it should be possible to merge now |
|
Any update regarding this one ? 👀 |
4 participants
This fixes the problems with the score raised in issue #1695 where a skip affects the score in non-daily habits, when it shouldn't.
To fix the issue, the part of the recompute function that calculates the rolling sum of completed habits in the last period was changed to take skips into account. A new function that calculates amount of skips in an interval is used to expand the length of that period.
Six tests where added, three to test the new helper function and three for testing the score.