Re: Question about the Chronology of the Interval class

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Question about the Chronology of the Interval class

Bård Dybwad Kristensen
Hi

The class Interval basically consists of three fields:

   /** The chronology of the interval */
   private volatile Chronology iChronology;
   /** The start of the interval */
   private volatile long iStartMillis;
   /** The end of the interval */
   private volatile long iEndMillis;


and a lot of different ways of constructing the object. What I find strange is that there is only one Chronology. I think there should be two Chronologies, one for the start and one for the end. They will be the same for all ways of constructing the object except for the constructor
public Interval(ReadableInstant start, ReadableInstant end),
 where each argument has it's own chronology. I really think that these two chronologies should be stored in the object, so that it is possible to get the start and end with their original chronologies from the interval at a later stage. As it is now, the chronology of the 'end' argument will be set to the same as for the 'start' argument.
Consider the code:

   @Test
   public void intervalTest() {
        DateTime start = new LocalDateTime("2011-01-01T12:30:00").toDateTime(DateTimeZone.forID("Europe/Paris"));
        DateTime end = new LocalDateTime("2011-01-01T18:30:00").toDateTime(DateTimeZone.forID("Asia/Dubai"));
       Interval interval = new Interval(start, end);
       System.out.println("start:\t\t\t" + start);
       System.out.println("end:\t\t\t" + end);
       System.out.println("interval.getStart():\t" + interval.getStart());
       System.out.println("interval.getEnd():\t" + interval.getEnd());
   }

Result:

start: 2011-01-01T12:30:00.000+01:00
end: 2011-01-01T18:30:00.000+04:00
interval.getStart(): 2011-01-01T12:30:00.000+01:00
interval.getEnd(): 2011-01-01T15:30:00.000+01:00

The fact that the end date time originally had TimeZone "Asia/Dubai" is gone forever.

Should it really be this way?

Any comments, anyone?

regards,
Bård Dybwad Kristensen


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2dcopy1
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Question about the Chronology of the Interval class

jodastephen
Its a simplification that I think leads to a clearer API overall. Of
course thats debateable, but I think this use case is pretty small to
complicate the whloe API with. Plus, I wouldn't change it anyway as it
would be backwards incompatble.

Stephen


On 28 September 2011 18:05, Bård Dybwad Kristensen <[hidden email]> wrote:

> Hi
>>
>> The class Interval basically consists of three fields:
>>
>>    /** The chronology of the interval */
>>    private volatile Chronology iChronology;
>>    /** The start of the interval */
>>    private volatile long iStartMillis;
>>    /** The end of the interval */
>>    private volatile long iEndMillis;
>>
>> and a lot of different ways of constructing the object. What I find
>> strange is that there is only one Chronology. I think there should be two
>> Chronologies, one for the start and one for the end. They will be the same
>> for all ways of constructing the object except for the constructor
>> public Interval(ReadableInstant start, ReadableInstant end),
>>  where each argument has it's own chronology. I really think that these
>> two chronologies should be stored in the object, so that it is possible to
>> get the start and end with their original chronologies from the interval at
>> a later stage. As it is now, the chronology of the 'end' argument will be
>> set to the same as for the 'start' argument.
>> Consider the code:
>>
>>    @Test
>>    public void intervalTest() {
>>         DateTime start = new
>> LocalDateTime("2011-01-01T12:30:00").toDateTime(DateTimeZone.forID("Europe/Paris"));
>>         DateTime end = new
>> LocalDateTime("2011-01-01T18:30:00").toDateTime(DateTimeZone.forID("Asia/Dubai"));
>>        Interval interval = new Interval(start, end);
>>        System.out.println("start:\t\t\t" + start);
>>        System.out.println("end:\t\t\t" + end);
>>        System.out.println("interval.getStart():\t" + interval.getStart());
>>        System.out.println("interval.getEnd():\t" + interval.getEnd());
>>    }
>>
>> Result:
>> start: 2011-01-01T12:30:00.000+01:00
>> end: 2011-01-01T18:30:00.000+04:00
>> interval.getStart(): 2011-01-01T12:30:00.000+01:00
>> interval.getEnd(): 2011-01-01T15:30:00.000+01:00
>> The fact that the end date time originally had TimeZone "Asia/Dubai" is
>> gone forever.
>> Should it really be this way?
>> Any comments, anyone?
>> regards,
>> Bård Dybwad Kristensen
>
> ------------------------------------------------------------------------------
> All the data continuously generated in your IT infrastructure contains a
> definitive record of customers, application performance, security
> threats, fraudulent activity and more. Splunk takes this data and makes
> sense of it. Business sense. IT sense. Common sense.
> http://p.sf.net/sfu/splunk-d2dcopy1
> _______________________________________________
> Joda-interest mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/joda-interest
>
>

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2dcopy1
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Question about the Chronology of the Interval class

Bård Dybwad Kristensen
Hi

Thanks for your answer.

I really think the clearest API would be if the Interval class only had two "main" getters:

DateTime getStart(); //Returning the start with original chronology
DateTime getEnd(); //Returning the end with original chronology

and no method:
Chronology getChronology();

Internally you could have 4 fields

    /** The chronology of start */
    private volatile Chronology startChronology;
    /** The chronology of end */
    private volatile Chronology endChronology;
    /** The start of the interval */
    private volatile long iStartMillis;
    /** The end of the interval */
    private volatile long iEndMillis;

This would be all the public getters needed.

Other getter as:
long getStartMillis()
long getEndMillis()
LocalDateTime getStartLocalDateTime()
LocalDateTime getEndLocalDateTime()
or whatever could of course also be made available.

With this the Interval class could do all it does today, and (IMHO) have an even simpler API.

As it is today, where the end will get the chronology of start, the Interval class yields some strange results is some cases. Consider the test:

    final DateTime time1 = new DateTime(2010, 1, 1, 12, 00, DateTimeZone.forOffsetHours(1));
    final DateTime time2 = new DateTime(2010, 1, 2, 12, 00, DateTimeZone.forOffsetHours(2));
    final DateTime time3 = new DateTime(2010, 1, 3, 12, 00, DateTimeZone.forOffsetHours(3));
    final DateTime time4 = new DateTime(2010, 1, 4, 12, 00, DateTimeZone.forOffsetHours(4));

    @Test
    public void testOverlap_total_overlap() {
        Interval a = new Interval(time1, time4);
        Interval b = new Interval(time2, time3);
        //This assertion is actually false, which it should not be.
//        assertEquals(new Interval(time2, time3), a.overlap(b));
        assertEquals(new Interval(time2, time3), b.overlap(a));
    }
    
    @Test
    public void testOverlap_overlap_different_start_same_end() {
        Interval a = new Interval(time2, time3);
        Interval b = new Interval(time1, time3);
        assertEquals(new Interval(time2, time3), a.overlap(b));
        //This assertion is actually false, which it should not be.
//        assertEquals(new Interval(time2, time3), b.overlap(a));
    }

    @Test
    public void testOverlap_overlap_same_start_different_end() {
        Interval a = new Interval(time1, time2);
        Interval b = new Interval(time1, time3);
        //In this case a.overlap(b) is actually the same as b.overlap(a)
        assertEquals(new Interval(time1, time2), a.overlap(b));
        assertEquals(new Interval(time1, time2), b.overlap(a));
    }


This shows that a.overlap(b) is equal to b.overlap(a) in some cases (most of them of course) but sometimes not. And they really should be equal all the time, right?
I guess the same flaw exists for the method gap().

But as you say; backwards compatibility prevents you from doing anything with this even if you agree with me. So in our project I had to write an extension/delegation class to handle this problem, as it is a real problem in our code. 


But hopefully you can consider this for future releases if possible.

regards,
Bård Dybwad Kristensen

On Thu, Sep 29, 2011 at 11:36, Stephen Colebourne <[hidden email]> wrote:
Its a simplification that I think leads to a clearer API overall. Of
course thats debateable, but I think this use case is pretty small to
complicate the whloe API with. Plus, I wouldn't change it anyway as it
would be backwards incompatble.

Stephen


On 28 September 2011 18:05, Bård Dybwad Kristensen <[hidden email]> wrote:
> Hi
>>
>> The class Interval basically consists of three fields:
>>
>>    /** The chronology of the interval */
>>    private volatile Chronology iChronology;
>>    /** The start of the interval */
>>    private volatile long iStartMillis;
>>    /** The end of the interval */
>>    private volatile long iEndMillis;
>>
>> and a lot of different ways of constructing the object. What I find
>> strange is that there is only one Chronology. I think there should be two
>> Chronologies, one for the start and one for the end. They will be the same
>> for all ways of constructing the object except for the constructor
>> public Interval(ReadableInstant start, ReadableInstant end),
>>  where each argument has it's own chronology. I really think that these
>> two chronologies should be stored in the object, so that it is possible to
>> get the start and end with their original chronologies from the interval at
>> a later stage. As it is now, the chronology of the 'end' argument will be
>> set to the same as for the 'start' argument.
>> Consider the code:
>>
>>    @Test
>>    public void intervalTest() {
>>         DateTime start = new
>> LocalDateTime("2011-01-01T12:30:00").toDateTime(DateTimeZone.forID("Europe/Paris"));
>>         DateTime end = new
>> LocalDateTime("2011-01-01T18:30:00").toDateTime(DateTimeZone.forID("Asia/Dubai"));
>>        Interval interval = new Interval(start, end);
>>        System.out.println("start:\t\t\t" + start);
>>        System.out.println("end:\t\t\t" + end);
>>        System.out.println("interval.getStart():\t" + interval.getStart());
>>        System.out.println("interval.getEnd():\t" + interval.getEnd());
>>    }
>>
>> Result:
>> start: 2011-01-01T12:30:00.000+01:00
>> end: 2011-01-01T18:30:00.000+04:00
>> interval.getStart(): 2011-01-01T12:30:00.000+01:00
>> interval.getEnd(): 2011-01-01T15:30:00.000+01:00
>> The fact that the end date time originally had TimeZone "Asia/Dubai" is
>> gone forever.
>> Should it really be this way?
>> Any comments, anyone?
>> regards,
>> Bård Dybwad Kristensen
>
> ------------------------------------------------------------------------------
> All the data continuously generated in your IT infrastructure contains a
> definitive record of customers, application performance, security
> threats, fraudulent activity and more. Splunk takes this data and makes
> sense of it. Business sense. IT sense. Common sense.
> http://p.sf.net/sfu/splunk-d2dcopy1
> _______________________________________________
> Joda-interest mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/joda-interest
>
>

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2dcopy1
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2dcopy1
_______________________________________________
Joda-interest mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/joda-interest
Loading...