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

Add / subtract workdays #1947

Closed
daaain opened this issue Sep 26, 2014 · 21 comments
Closed

Add / subtract workdays #1947

daaain opened this issue Sep 26, 2014 · 21 comments

Comments

@daaain
Copy link

daaain commented Sep 26, 2014

Hey,

I needed a way to step over weekends and created a small helper method to achieve this.

Tried to make it into a pull request, but found the code a bit intimidating on the first sight, so thought I should ask first if this is something that has its place in the library?

Ideally I guess it should be available as moment.add(2, 'workdays') or so.

Let me know if this is useful or maybe already covered somehow?

JSfiddle demo: http://jsfiddle.net/dain/5xrr79h0/

  moment.fn.addWorkdays = function (days) {
    var increment = days / Math.abs(days);
    var date = this.clone().add(Math.floor(Math.abs(days) / 5) * 7 * increment, 'days');
    var remaining = days % 5;
    while(remaining != 0) {
      date.add(increment, 'days');
      if(date.isoWeekday() !== 6 && date.isoWeekday() !== 7)
        remaining -= increment;
    }
    return date;
  };
@icambron
Copy link
Member

Always good to ask first. A few thoughts:

  • One issue is that the weekend isn't actually the same everywhere. Many muslim countries do Thurs-Fri, for example. We could add that to the localization settings, but then we'd have to go through all of them and add it, so that's unfortunate.
  • "Workdays" sort of implies it skips holidays too, right?

@daaain
Copy link
Author

daaain commented Sep 28, 2014

Right, I didn't dare to go into the subtleties of the ever changing holiday patterns in each country and I didn't even know that weekends can be on totally different days!

Going for the whole hog would be a lot of work as you say, so I'm wondering if there was an interface to optionally plug in holidays and unusual weekend patterns would be a good enough step forward?

My solution in its current form is already solving a few common gotchas (using isoWeekday to side step which day the week starts on, only using a loop for the last few days to improve performance and supporting going both directions in time), so better than having people work this out from scratch.

Actually, maybe this should just be a plugin after all?

@icambron
Copy link
Member

The general pattern in Moment would be that the locale-specific options load in automatically. It might not be too much work to use the CLDR data to find the locales and just plug the weekends into the right files [1]. Having them be loaded in as options would make it hard for people to just switch locales and have it work. OTOH, having holidays be optionally loaded in seems reasonable.

I do think this is probably best off as a plugin. I can see it being enormously useful to a lot of people, but kind of problematic to support in Moment's core. What do you think, @ichernev?

[1] One day, we'll generate the locale files from CLDR...one day.

@ichernev
Copy link
Contributor

ichernev commented Oct 7, 2014

Weekday handling has been requested before. As @icambron pointed out differences in cultures and year-to-year differences in (national) holidays are the biggest issues. CLDR has weekend data. But handling holidays is harder. I just found that http://timeanddate.com have very rich holiday data -- I sent them an email to see if we can arrange something :)

Other than that -- if we put holiday information from CLDR (just weekends), than we can support add(N, 'weekdays'), add(N, 'weekenddays'), same for subtract and diff. Later on if we add data for holidays we can package it in a plugin and also add add(N, 'holidays'), add(N, 'workdays') and friends. I guess we can also add some accessors: is('holiday'/'weekend') etc.

The algorithm for adding/subtracting (without holidays) should be pretty straight forward and work in constant time, the one outlined here is taking O(N).

@jamesplease
Copy link

The algorithm for adding/subtracting (without holidays) should be pretty straight forward and work in constant time, the one outlined here is taking O(N).

I worked on this last night. My solution is as follows:

  • Finding the number of workdays, or weekends, is a composition of a more fundamental problem
  • The fundamental problem is "How many of a particular day exist between two days?"
  • More abstractly, the fundamental problem is "How many values of a periodic function exist within some interval?"

Each day of the week is a value of a periodic function with period 7.

In light of that, finding the weekends (in the USA) is solved by:

"How many Saturdays are in this interval AND how many Sundays are in this interval?"

The week days, excluding Holidays, is the inverse operation.

This problem could be generalized to any other regular system, such as cultures with a different set of weekend days.

Not sure if this would help anyone, but it's the solution I'm going with in my app. I should have it abstracted into a library in the next day or two.

Libraries:

  • nearest-periodic-value.js
    This assumes the point and value are not within 1 period of eachother. It's solved for a more general use case than what moment needs, but it's 1 LoC so nbd.
  • contained-periodic-values.js
    Gets you the # of contained periodic values in an interval. Operates on an inclusive start, exclusive end interval, so it plays well with moment calculations.

@jamesplease
Copy link

@ichernev is this the correct fixture data for add/subtract?

https://gist.github.com/jmeas/0a713e0b9901a25a32f1

I made the end dates exclusive, but I'm thinking this may not be correct. e.g.; Sunday + 1 = Tuesday in my fixtures.

I assumed my data was wrong, so I updated it and released v1 of moment-business with constant time solutions for the methods it provides. @ichernev lemme know if there's anything I can do to help this land in master.

@ioleo
Copy link

ioleo commented Mar 4, 2015

Thanks @daaain. Based on your solution I've created this gist with add/subtract(x, 'workdays')and isWorkday(). Uses moment.easter (included in gist). Polish National Holidays are included, but you can easily modify script to add your own.

@jamesplease
Copy link

@loostro, your add/subtract algorithms will be substantially slower than the constant-time solutions in moment-business when adding large numbers of days.

@ioleo
Copy link

ioleo commented Mar 4, 2015

@jmeas in my current use case, thats not a problem, as i need to "add" or "subtract" 2 workdays only, but it would be nice to see a better solution

@jamesplease
Copy link

as i need to "add" or "subtract" 2 workdays only,

Ah, okay.

it would be nice to see a better solution

I linked to one in the above comment 😛

@ioleo
Copy link

ioleo commented Mar 4, 2015

@jmeas you mean moment.fn.addWeekDays ? or the libraries from previous posts?

@jamesplease
Copy link

you mean moment.fn.addWeekDays ? or the libraries from previous posts?

Both. moment.fn.addWeekDays is from a library in a previous post, moment-business.

@ioleo
Copy link

ioleo commented Mar 4, 2015

ah.. i see :) i'll study your solution and fix my gist for Polish workdays to be more efficient, thanks!

@amakhrov
Copy link

This is how a similar problem (getting difference in week days) is solved in the Carbon library (PHP):
diffFiltered() method: link
diffInWeekdays link (basically a wrapper over diffFiltered)

This interface allows a developer to specify a custom filter function - thus enabling to use the library with arbitrary weekends/holidays

@jamesplease
Copy link

jamesplease commented May 10, 2016

Here's a working constant time solution to this problem for western work weeks:

moment-business

atm it's hard coded to support the western work week, but the underlying algorithms should be generic enough to be tweaked to support any weekend, so long as it's periodic (repeats week to week) and simple (every week is structured the same in terms of what's a week and what's not a week; it doesn't go back and forth between two patterns, for instance)

If any of the maintainers of moment want to incorporate any of this work into moment, lmk. I'd be happy to hep!

@SuperManfred
Copy link

Instead of hard coded western work week and weekend, wouldn't it be easier for moment to incorporate if easily configurable workdays and non-workdays, and easy to set work time hh:mm

@jamesplease
Copy link

Most likely, which is one of the reasons why I made the underlying algo's in moment-business accept any periodic pattern of "holes" (weekends) along an interval (a week). This should make it fairly simple to build an API to work with configurable work days and non-work days. I didn't put in that effort because I only needed western work week support for my use case.

I never made a PR into moment directly because I'd prefer there be some confirmation from the core team that the PR wouldn't just get closed, but I never heard back.

@icambron
Copy link
Member

@jmeas seems like it makes the most sense as a plugin, just as you have it. We'd be happy to list it under the plugins section of the docs you make a PR to that effect on the docs repository.

@maggiepint
Copy link
Member

To me, this is a permanent plugin. We don't have the ability to determine work days for every locale, especially including holidays. I'm going to close this one.

@jamesplease
Copy link

Thanks for the updates @icambron & @maggiepint !

@stevenlovegrove
Copy link

FYI, @daaain 's method is also O(1) - it may look as though it iterates over all days between the start and end, but it does not. It only iterates over the modulo which is bounded < 5.

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

No branches or pull requests

10 participants