Minor fail with date comparison
-
I found this (anonymized) method in our production code:
//inputDate and anchorDate are converted to local time with the time portion stripped off boolean isNotPrior(DateTime inputDate, DateTime anchorDate)
{
return inputDate.plus(Seconds.ONE).isAfter(anchorDate);
}I would have thought it would be simpler and more accurate to just use
return !inputDate.isBefore(anchorDate);
.
Also, in our codebase, instead of a parallelisNotSubsequent()
method, this method is re-used with the passed parameters switched around.
-
@djls45 said in Minor fail with date comparison:
Also, in our codebase, instead of a parallel isNotSubsequent() method, this method is re-used with the passed parameters switched around.
Oh great, all that extra complexity for someone to comprehend. Bonus points if
isAfter
,isNotPrior
andisNotSubsequent
are all used in the same class somewhere. Triple bonus if they're in the same expression…
-
@dkf said in Minor fail with date comparison:
@djls45 said in Minor fail with date comparison:
Also, in our codebase, instead of a parallel isNotSubsequent() method, this method is re-used with the passed parameters switched around.
Oh great, all that extra complexity for someone to comprehend. Bonus points if
isAfter
,isNotPrior
andisNotSubsequent
are all used in the same class somewhere. Triple bonus if they're in the same expression…Here is another small nugget from the same class:
It does use bothboolean isWithinRange(DateTime inputDate, DateTime anchorDate, DayRange days)
{
boolean result = true;
DateTime testDate = anchorDate.toDateMidnight().toDateTime();
if(days.getBefore() != null)
{
DateTime beforeDate = testDate.minusDays(days.getBefore()).minus(Seconds.ONE);
result = inputDate.isAfter(beforeDate);
}
if(days.getAfter() != null)
{
DateTime afterDate = testDate.plusDays(days.getAfter() + 1);
result = result && inputDate.isBefore(afterDate);
}
return result;
}isBefore()
andisAfter()
fromDateTime
, but notisNotPrior()
. I'm not sure why there's so much manipulation of the dates for the comparisons, when they are passed in as midnight local time in all uses. (Notice the superfluous conversion to midnight on the second line.) Perhaps it's due to code creep, with no refactoring for efficiency/clarity going on...?
-
@djls45 said in Minor fail with date comparison:
Seconds.ONE
Do you also have Seconds.FIFTY_TWO_AND_THREE_QUARTERS?
-
What's with the names:
isNotPrior()
,isNotSubsequent()
Someone into not logic? I mean, think about it:
if (!isNotPrior(date1,date2)) { ... }
Even I was able to come up with better names than that:
precedes()
,succeeds()
-
@dkf said in Minor fail with date comparison:
Bonus points if
isAfter
,isNotPrior
andisNotSubsequent
are all used in the same class somewhere. Triple bonus if they're in the same expression…Quadruple if they're all implemented differently....
@CoyneTheDup said in Minor fail with date comparison:
Even I was able to come up with better names than that:
precedes()
,succeeds()
proceeds()
,concedes()
?
-
@PJH said in Minor fail with date comparison:
@CoyneTheDup said in Minor fail with date comparison:
Even I was able to come up with better names than that:
precedes()
,succeeds()
proceeds()
,concedes()
?For date comparisons? Not likely.
Also, you forgot:
concedes()
,intercedes()
,secedes()
,recedes()
,Mercedes()
Every date utility class needs all of those, right?
-
@CoyneTheDup said in Minor fail with date comparison:
What's with the names:
isNotPrior()
,isNotSubsequent()
I came up with
isNotSubsequent()
to go withisNotPrior()
. It's not actually in this class.Someone into not logic? I mean, think about it:
if (!isNotPrior(date1,date2)) { ... }
In this case, the code uses
if(isNotPrior(date2,date1))
.Even I was able to come up with better names than that:
precedes()
,succeeds()
The proper usage would be to use the built-in functionality of
DateTime
:
date1.isBefore(date2)
,date1.isAfter(date2)
,!date1.isBefore(date2)
,!date1.isAfter(date2)
.
-
@CoyneTheDup said in Minor fail with date comparison:
Also, you forgot: […] Mercedes()
Every date utility class needs all of those, right?http://media2.whosaystatic.com/231127/231127_800wc.jpg
Yes.