Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added TemporalAdjuster for rounding to nearest part of an hour #196

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

raupachz
Copy link
Contributor

Hi there,

this PR contains four TemporalAdjuster to round time to the nearest part of an hour. Rounding to the nearest part of an hour is often done in payroll systems, scheduling and appointment making.

  • Add public static TemporalAdjuster nearestHalf to Temporals
  • Add public static TemporalAdjuster nearestQuarter to Temporals
  • Add public static TemporalAdjuster nearestTenth to Temporals
  • Add public static TemporalAdjuster nearestTwelfth to Temporals

The code is repetitive and contains hardcoded numbers. Granted not the most clever approach, but it is straightforward and easy to read and maintain.

I know this project emphasizes well written JavaDoc. I am not a native speaker. I tried.

@jodastephen
Copy link
Member

I'm happy with the concept that adjusters could be added for use cases like these. However the implementation, as a series of if/else is not desirable. Plus, thus methods are not "nearest", but "truncate".

What needs implementing is a single adjuster with a parameter that indicates the amount to truncate to. This can be exposed publicly, eg truncated(Duration). The named adjusters are then just a static instance of the implementation, eg truncatedHalfHour().

There might separately be a case for HALF_UP rounding nearestHalfHour() etc.

@raupachz
Copy link
Contributor Author

Thanks for the feedback Stephen!

Could you elaborate on why the methods are truncate instead of nearest? I thought truncate means losing a part of something. Dropping everything below minutes for example. What I am trying to achieve is rounding towards the nearest/closest part of an hour. Given 17:27 the next quarter would be 17:30. Given 17:33 the next quarter again is 17:30. This of often done in payroll or scheduling.

@Clockwork-Muse
Copy link
Contributor

... why does the implementation advance an entire day, instead of rounding to the hour? You also have so many test cases they become noise (you mostly only need ones around the transition boundaries).

This of often done in payroll

This is often wage theft from employees. Modern timekeeping systems should record actual clock-in/-out times and work from that.

@raupachz
Copy link
Contributor Author

raupachz commented Dec 29, 2021

... why does the implementation advance an entire day, instead of rounding to the hour? You also have so many test cases they become noise (you mostly only need ones around the transition boundaries).

That is a mistake done on my part. Advancing to the next day should only happen if one rounds from 23:50 to midnight on the next day. Thanks for pointing this out.

This is often wage theft from employees. Modern timekeeping systems should record actual clock-in/-out times and work from that.

In theory you are correct, but unfortunately this is not how its done in practice. I am not a lawyer, but I would not call it theft. For example if I clock in at 7:59 and it gets rounded to 8:00 then yes I lose a minute, but if one clocks out at say 17:40 and the time gets rounded to 17:45 you get five minutes. But again, I am not a lawyer.

@jodastephen
Copy link
Member

Maybe I misread the code. "nearest" should, IMO, mean the same as HALF_UP rounding.

@raupachz
Copy link
Contributor Author

Ok, different approach. Thank you for the discussion. I understand that "nearest" was not a good idea. How about calling it rounding and introducing a rounding mode?

public static TemporalAdjuster roundTime(Duration duration, RoundingMode roundingMode)

Duration in minutes must be a divisor of 60, since there are sixty minutes in an hour and we want to divide the hour into equal parts.

The rounding mode is one of UP, DOWN or HALF_UP.

  • DOWN rounds time down towards the last part.
  • UP rounds time to the next part. This could result in adding an hour to the result.
  • HALF_UP rounds towards the "nearest" part. UP if the distance is equal. Again, this could result in adding an hour to the result.

I think with the added rounding mode we can fit several use cases. In scheduling you often want to refer to the next quarter of half an hour. And yes in time keeping - I agree this is controversial - you might need to round HALF_UP.

@perceptron8
Copy link
Contributor

How about durations like PT45M (32 × PT45M = PT24H) or PT100S (36 × PT100S = PT1H)? Why would they have to be invalid?

Also, please note that "starting point" may be other than full hour (and possibly expressed in something other than local time). I mean intervals similar to "07:30 + n × PT20M" and use cases like "run this task every P1D12H starting at 2001-01-01T06:00:00Z".

Maybe something like

public static TemporalAdjuster roundTime(Temporal start, Duration duration, RoundingMode roundingMode)

could be more useful?

@raupachz
Copy link
Contributor Author

raupachz commented Jan 6, 2022

Thanks for the feedback @perceptron8

How about durations like PT45M (32 × PT45M = PT24H) or PT100S (36 × PT100S = PT1H)? Why would they have to be invalid?

Not saying they are invalid. I only looked at rounding towards a fraction of the hour. Did not take a whole day into consideration.

Also, please note that "starting point" may be other than full hour (and possibly expressed in something other than local time). I mean intervals similar to "07:30 + n × PT20M" and use cases like "run this task every P1D12H starting at 2001-01-01T06:00:00Z".

Agreed. That is nice to have, but isn't this more cron-ish? I think this would go into a different direction than the problem I am trying to solve.

Anyways, I did clean up the code and added a bit more documentation. Not saying I am done. Does it look like I am going into the right direction?

@jodastephen
Copy link
Member

This all gets complicated because there are different ways to look at the problem.

  • what is being rounded? the hour, hour-min, hour-min-sec?
  • the subdivision? eg. rounding to 30 mins, 15 mins, 5 mins
  • how to round? up, down nearest

I believe the first two can be handled with a single Duration parameter. This means the method is not only limited to hour-min rounding.

The method name(s) need to flow when imported. I think three separate methods probably makes this clearest:

  • `time.with(roundedNearest(Duration.ofHours(1));
  • `time.with(roundedUp(Duration.ofHours(1));
  • `time.with(roundedDown(Duration.ofHours(1));

The duration would need to be divisible into a day. The implementation would work based off the nano-of-day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants