Thanks for the patch. The docs say that if the end date is before the start date, the stream is empty. While I can see just about why LongStream.range() works that way, I don't think this API should. The minimum is an exception, but it would be easy to support negative in the days-only case or the months-only case. The problem is where there are both months/years and days and those should be exceptions.
The single-arg implementation uses plusDays() with an incrementing number. When the performance patch goes in, the proposed streaming implementation will be optimal for small streams but less optimal for large ones. The fastest way to loop over a list of dates would be to manually generate them by incrementing the day until it exceeds the length of month, and so on. However, this would be serial. As such, I think the best way to write this, taking account of how plusDays() is implemented, is as follows: LongStream.range(start.toEpochDay(), end.toEpochDay()).mapToObj(LocalDate::ofEpochDay); This ensures that toEpochDay() is only called twice, rather than almost every time (within plusDays). In the period-based method, it would be best to capture the case where totalMonths == 0 and days > 0 and forward to another method that ignores months. That private method can share implementation with the public single-arg method (passing in 1). The docs for the period-based method should specify the strategy that produces the results (in abstract terms). This needs to cover that the result is equivalent to always adding to the start period a multiple of the period. I haven't looked at the maths in detail at this point. Hopefully the test cases cover it. Some nits: I prefer to avoid @link in the first sentence. Just using 'stream' would be sufficient. The first sentence should be a summary. In this case it probably has a bit too much detail. The @return has 'values' on a new line when it could be on the same line. If statements need braces. thanks Stephen On 26 December 2015 at 16:29, Tagir F. Valeev <amae...@gmail.com> wrote: > Hello! > > Thank you for your comments. I logged an issue: > https://bugs.openjdk.java.net/browse/JDK-8146218 > > And prepared a patch: > http://amaembo.github.io/cr/8146218/webrev-1/ > (sorry for not using official code-review server, I cannot access it > currently, hopefully this problem will be solved soon). Please review, > especially the specification: English is not my native language. > > The patch introduces two methods in LocalDate class: > datesUntil(LocalDate endExclusive) and > datesUntil(LocalDate endExclusive, Period step). > > The implementation of the former one is just a one-liner. I decided to > produce an empty stream if endExclusive is before than this to stay in > sync with IntStream.range/LongStream.range. > > The latter method is more tricky. It could be implemented using the > takeWhile() method, but I wanted to make the sized stream, so it can > parallelize perfectly and its count method is guaranteed to work in > constant time (see JDK-8067969). Thus it's necessary to precompute > exactly the size of the stream. > > Zero and negative periods are not supported. On the one hand it might > be useful in some scenarios (assuming the ending date is before the > start date). On the other hand it's possible to define some weird > period like Period.of(0,1,-30) which may go in both directions > depending on current date. Period like Period.of(400,0,-146097) is > equivalent to ZERO, so it would never advance, but isZero would return > false (and even normalization will not help). Theoretically it's > possible to handle all such cases, but it would make the method and > its specification much more complex. Thus I would like to disable > negative periods at all. Though I'm open to discussion. > > Probably I added too many test cases. If it poses the problem I may > try to reduce the number of tests. > > With best regards, > Tagir Valeev. > > SC> JSR-310 was developed in parallel with the Stream API. While we > SC> briefly considered adding some stream methods, it wasn't obvious what > SC> would be of value. > > SC> A key question is whether the methods are either specific to a narrow > SC> use case or more general. Specific methods like months() on Year are > SC> very limited. You'd need: > SC> Year::months() > SC> Year::dates() > SC> YearMonth::dates() > SC> Only three methods, so not too bad. There would be some use cases, but > SC> enough to include them? Not sure. > > SC> A method that provides all days between two dates: > SC> Stream<LocalDate> datesUntil(LocalDate endExclusive); > SC> would have some value as it is more common and trickier to write. > > SC> A method that provides Period addition looping might make more sense: > SC> Stream<LocalDate> datesUntil(LocalDate endExclusive, Period step); > SC> This is actually quite hard to write correctly, as it requires > SC> multiplying the period by the loop index, not adding the period each > SC> time (to handle end-of-month effects). > > SC> But where to stop? > SC> Stream<Instant> instantsUntil(Instant endExclusive, Duration step); > SC> Stream<ZonedDateTime> instantsUntil(Instant endExclusive, Duration > step); > SC> Stream<OffsetDateTime> instantsUntil(Instant endExclusive, Duration > step); > SC> ... > > > SC> I think the place to start is the two methods on LocalDate which are > SC> tricky enough to be justified. Then perhaps the methods on > SC> Year/YearMonth. > > SC> Stephen > > > SC> On 10 December 2015 at 16:31, Tagir F. Valeev <amae...@gmail.com> > wrote: > >> - in class Year: > >> > >> // Returns sequential ordered stream of all months within this Year: > >> Stream<YearMonth> months(); > >> // Returns sequential ordered stream of all days within this Year: > >> Stream<LocalDate> days(); > >> // Returns sequential ordered stream of all years starting from this > >> // Year until the supplied year, exclusive > >> Stream<Year> yearsUntil(Temporal endExclusive); > >> > >> - in class YearMonth: > >> > >> // Returns sequential ordered stream of all days within this YearMonth: > >> Stream<LocalDate> days(); > >> // Returns sequential ordered stream of all months starting from this > >> // YearMonth until the supplied YearMonth, exclusive > >> Stream<YearMonth> monthsUntil(Temporal endExclusive); > >> > >> - in class LocalDate: > >> > >> // Returns sequential ordered stream of all months starting from this > >> // LocalDate until the supplied LocalDate, exclusive > >> Stream<LocalDate> daysUntil(Temporal endExclusive); > >> > >> The implementation of these methods could be quite simple. For example: > >> > >> class Year { > >> public Stream<LocalDate> days() { > >> return IntStream.rangeClosed(1, length()).mapToObj(this::atDay); > >> } > >> } > >> > >> What do you think? > >> > >> With best regards, > >> Tagir Valeev. > >> > >
