ShoppingCart.getPartyDaysSinceCreated

[Adam Heath-2]

The above method makes use of a BigDecimal.  This is wrong.  It should
be an int.  Originally, before the BigDecimal conversion, it was a
Double.  This method should have never been converted.

Also, it does time internal calculation wrong.  It will have the same
daylight savings time problem, when now is after DST, and the
createdDate is before DST.  The time will be 1 hour short, which will
get rounded down to the next whole day.

Is there any existing logic to calculate the number of whole days
between 2 times?

[Scott Gray-2]

On 26/03/2010, at 5:13 PM, Adam Heath wrote:

> The above method makes use of a BigDecimal.  This is wrong.  It should
> be an int.  Originally, before the BigDecimal conversion, it was a
> Double.  This method should have never been converted.

It looked like a good candidate at the time and looking at it now, it still looks like a good candidate for BigDecimal.  Why should it never have been converted?

[Adam Heath-2]

Scott Gray wrote:
> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
>
>> The above method makes use of a BigDecimal.  This is wrong.  It should
>> be an int.  Originally, before the BigDecimal conversion, it was a
>> Double.  This method should have never been converted.
>
> It looked like a good candidate at the time and looking at it now, it still looks like a good candidate for BigDecimal.  Why should it never have been converted?

Days is a whole number.  2 billion days is over 5.8 million years.
Why would it need to be a Double previously, and a BigDecimal now?

[Adrian Crum-2]

--- On Fri, 3/26/10, Adam Heath <[hidden email]> wrote:

> Scott Gray wrote:
> > On 26/03/2010, at 5:13 PM, Adam Heath wrote:
> >
> >> The above method makes use of a BigDecimal. 
> This is wrong.  It should
> >> be an int.  Originally, before the BigDecimal
> conversion, it was a
> >> Double.  This method should have never been
> converted.
> >
> > It looked like a good candidate at the time and
> looking at it now, it still looks like a good candidate for
> BigDecimal.  Why should it never have been converted?
>
> Days is a whole number.  2 billion days is over 5.8
> million years.
> Why would it need to be a Double previously, and a
> BigDecimal now?

Not to mention the method performs millisecond arithmetic - which is a no-no.

[Scott Gray-2]

On 26/03/2010, at 6:32 PM, Adam Heath wrote:

> Scott Gray wrote:
>> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
>>
>>> The above method makes use of a BigDecimal.  This is wrong.  It should
>>> be an int.  Originally, before the BigDecimal conversion, it was a
>>> Double.  This method should have never been converted.
>>
>> It looked like a good candidate at the time and looking at it now, it still looks like a good candidate for BigDecimal.  Why should it never have been converted?
>
> Days is a whole number.  2 billion days is over 5.8 million years.
> Why would it need to be a Double previously, and a BigDecimal now?

Days is not a whole number, 1.5 days is perfectly valid.  I have no idea why it needed to be a double previously but when you're converting thousands of doubles to BigDecimals then you don't dwell on each one for too long.

[Adam Heath-2]

Scott Gray wrote:

> On 26/03/2010, at 6:32 PM, Adam Heath wrote:
>
>> Scott Gray wrote:
>>> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
>>>
>>>> The above method makes use of a BigDecimal.  This is wrong.  It should
>>>> be an int.  Originally, before the BigDecimal conversion, it was a
>>>> Double.  This method should have never been converted.
>>> It looked like a good candidate at the time and looking at it now, it still looks like a good candidate for BigDecimal.  Why should it never have been converted?
>> Days is a whole number.  2 billion days is over 5.8 million years.
>> Why would it need to be a Double previously, and a BigDecimal now?
>
> Days is not a whole number, 1.5 days is perfectly valid.  I have no idea why it needed to be a double previously but when you're converting thousands of doubles to BigDecimals then you don't dwell on each one for too long.

I discovered this bug in an old system(2 years), that did repeat
billing using a shoppinglist that get's converted to a real
shoppingcart and order once a month.  I had a promo that would fire
after the activate the first time, giving the user one month free, and
 had a condition that made it valid for the first 28 days only.  This
failed this year, at this time, because feb 16 to mar 16 is actually
27 days, and 23 hours, which got rounded down to 27.

In the version I have, it is still a Double, but the algo doesn't do
an explicit cast, so integer arthmetic is being done, so it gets
rounded down.

In any event, this method should not have been a Double in the first
place, and I'm saying it both needs to be converted to an int, and the
calculation fixed.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/26/10, Adam Heath <[hidden email]> wrote:
>> Scott Gray wrote:
>>> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
>>>
>>>> The above method makes use of a BigDecimal.
>> This is wrong.  It should
>>>> be an int.  Originally, before the BigDecimal
>> conversion, it was a
>>>> Double.  This method should have never been
>> converted.
>>> It looked like a good candidate at the time and
>> looking at it now, it still looks like a good candidate for
>> BigDecimal.  Why should it never have been converted?
>>
>> Days is a whole number.  2 billion days is over 5.8
>> million years.
>> Why would it need to be a Double previously, and a
>> BigDecimal now?
>
> Not to mention the method performs millisecond arithmetic - which is a no-no.

I said as much in my first email.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/26/10, Adam Heath <[hidden email]> wrote:
>> Scott Gray wrote:
>>> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
>>>
>>>> The above method makes use of a BigDecimal.
>> This is wrong.  It should
>>>> be an int.  Originally, before the BigDecimal
>> conversion, it was a
>>>> Double.  This method should have never been
>> converted.
>>> It looked like a good candidate at the time and
>> looking at it now, it still looks like a good candidate for
>> BigDecimal.  Why should it never have been converted?
>>
>> Days is a whole number.  2 billion days is over 5.8
>> million years.
>> Why would it need to be a Double previously, and a
>> BigDecimal now?
>
> Not to mention the method performs millisecond arithmetic - which is a no-no.

Actually, the entityengine should be made to store an interval type,
using by any kind of repeat/period/calendar field, and this new
java-type should not have any type of conversion to raw numbers.

[Adrian Crum-2]

--- On Fri, 3/26/10, Adam Heath <[hidden email]> wrote:

> Adrian Crum wrote:
> > --- On Fri, 3/26/10, Adam Heath <[hidden email]>
> wrote:
> >> Scott Gray wrote:
> >>> On 26/03/2010, at 5:13 PM, Adam Heath wrote:
> >>>
> >>>> The above method makes use of a
> BigDecimal.
> >> This is wrong.  It should
> >>>> be an int.  Originally, before the
> BigDecimal
> >> conversion, it was a
> >>>> Double.  This method should have
> never been
> >> converted.
> >>> It looked like a good candidate at the time
> and
> >> looking at it now, it still looks like a good
> candidate for
> >> BigDecimal.  Why should it never have been
> converted?
> >>
> >> Days is a whole number.  2 billion days is
> over 5.8
> >> million years.
> >> Why would it need to be a Double previously, and
> a
> >> BigDecimal now?
> >
> > Not to mention the method performs millisecond
> arithmetic - which is a no-no.
>
> Actually, the entityengine should be made to store an
> interval type,
> using by any kind of repeat/period/calendar field, and this
> new
> java-type should not have any type of conversion to raw
> numbers.

That is what TimeDuration was intended for.