Is it better DX to return null if `time.isNaN()`, ...

# codereview

[Sam Stone]

Is it better DX to return null if time.isNaN(), or some special value like Date.from(Instant.MIN)?

/**
 * A method that returns a [Date] from the time passed in as a parameter.
 *
 * @param time
 * The time to be set as the time for the `Date`. The time expected is in the format: 18.75
 * for 6:45:00 PM.time is sunrise and false if it is sunset
 * @param isSunrise true if the
 * @return The Date.
 */
protected fun getDateFromTime(time: Double, isSunrise: Boolean): Date? {
    if (time.isNaN()) {
        return null
    }
    var calculatedTime: Double = time
    val adjustedCalendar: Calendar = adjustedCalendar
    val cal: Calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"))
    cal.clear() // clear all fields
    cal[Calendar.YEAR] = adjustedCalendar[Calendar.YEAR]
    cal[Calendar.MONTH] = adjustedCalendar[Calendar.MONTH]
    cal[Calendar.DAY_OF_MONTH] = adjustedCalendar[Calendar.DAY_OF_MONTH]
    val hours = calculatedTime.toInt() // retain only the hours
    calculatedTime -= hours.toDouble()

    // retain only the minutes
    calculatedTime *= 60
    val minutes = calculatedTime.toInt()
    calculatedTime -= minutes.toDouble()

    // retain only the seconds
    calculatedTime *= 60
    val seconds = calculatedTime.toInt()
    calculatedTime -= seconds.toDouble() // remaining milliseconds

    // Check if a date transition has occurred, or is about to occur - this indicates the date of the event is
    // actually not the target date, but the day prior or after
    val localTimeHours = (geoLocation.longitude / 15).toInt()
    if (isSunrise && localTimeHours.plus(hours) > 18)
        cal.add(Calendar.DAY_OF_MONTH, -1) 
    else if (!isSunrise && localTimeHours.plus(hours) < 6) 
        cal.add(Calendar.DAY_OF_MONTH, 1)
    cal[Calendar.HOUR_OF_DAY] = hours
    cal[Calendar.MINUTE] = minutes
    cal[Calendar.SECOND] = seconds
    cal[Calendar.MILLISECOND] = (calculatedTime * 1000).toInt()
    return cal.time
}

[Joffrey]

I guess the most important points about this code are: don't use Calendar, and don't use java.util.Date

[Adam S]

using Double to represent time like 6.45 is asking for trouble too

[Joffrey]

Why do you need a function like this in the first place? How did you end up with a time represented as a Double?

[Joffrey]

Another point is that this function looks like it could be pure from its API, but it really isn't. You are taking the day from somewhere else, and the time from the input, which seems strange. And also somehow the current geoLocation changes the result 🤔

[Sam Stone]

This is a library which needs to be backwards compatible to very early versions of Java. Of course the location changes the date. A time is effectively a LocalTime (without any time zone or geographical information). A Date represents a zoned date-time. One example of an input into this function is UTC sunrise. Complex date-time stuff. Part of a Kotlin port of this java library.

[Joffrey]

A Date represents a zoned date-time

No it doesn't. A java.util.Date represents an instant in time without zone information. So you need a time zone and a local date to convert a local time to a Date. The current geoLocation could be used to find the current time zone, but that doesn't seem to be what you're doing here.

[Joffrey]

This is a library which needs to be backwards compatible to very early versions of Java

How far back are we talking? Java 8 is the oldest version that's technically still supported (and it already had the java.time APIs)

EDIT: the library you linked has a target bytecode level of 1.8, so you're free to use java.time ;)

[Adam S]

so you’re converting this function from Java to Kotlin?

https://github.com/KosherJava/zmanim/blob/d24323b295779dd4922dccf418afc54e420e592a/src/main/java/com/kosherjava/zmanim/AstronomicalCalendar.java#L547-L550

protected Date getDateFromTime(double time, boolean isSunrise) {
    if (Double.isNaN(time)) {
        return null;
    }
    ...
}

In this instance I’d keep the same logic and return a nullable date, Date?. There’s usually no downside in returning null in Kotlin, because the type-system means a null isn’t going to ‘leak’ like it might in Java, and null-checks are super easy to perform.

Once everything was Kotin I’d refactor everything to use the new java.time classes, or kotlinx-datetime. And if the obsolete Java Date & related types were still necessary I’d introduce some helper functions in the public API that would still accept them, but convert them to the newer types, but only if that was really necessary! I might even create a new library to isolate the older types, to keep the core library ‘clean’, and use @Deprecate and ReplaceWith to drive home the point.

Speaking generally, whether to return null vs a default vs throwing an exception really depends on the situation. Where is the data coming from, and where will it go?

• If the provided data is coming from a user input, I’d throw an exception. It’s best to make sure that bad data doesn’t enter the system ASAP
• Returning null also performs the same operation, but is more lenient. It can also ‘defer’ throwing the exception - for example, if the data is being put into a database which requires the data is not-null, then the exception could be thrown then.
• using a default value might sometimes be used rarely, but I would be very cautious. In your case there’s a big risk of introducing data that is valid, but nonsensical.

[Sam Stone]

@Adam S well said.