Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preconditions for validating Duration instances #151

Open
kluever opened this issue Jan 28, 2020 · 4 comments
Open

Preconditions for validating Duration instances #151

kluever opened this issue Jan 28, 2020 · 4 comments
Labels

Comments

@kluever
Copy link
Contributor

kluever commented Jan 28, 2020

We have the following precondition-style utilities inside of google:

Durations.checkNotNegative(duration)
Durations.checkPositive(duration)

They throw IllegalArgumentException if the duration is negative or not positive (respectively). Negative durations often are a bug, so it's nice to validate Duration parameters like RPC timeouts or cache expirations. E.g.,

public final class Frobber {
  private final Duration timeout;
  public Frobber(Duration timeout) {
    this.timeout = checkPositive(timeout);
  }
}

Any chance we could find a home for these in threeten-extra?

@jodastephen
Copy link
Member

jodastephen commented Feb 18, 2020

And yes, this is an ideal addition.

The ThreeTen-Extra home for utilities is Temporals. While we could create an Amounts or Durations, I think Temporals would do just fine.

(If you can think of other validation-type things, then perhaps a TemporalValidation class might make sense, but when it is just two methods, the existing utils works OK.)

@kluever
Copy link
Contributor Author

kluever commented Feb 18, 2020

We have other things in our Durations static utility class if that helps make a case for a whole separate class? These just probably aren't as useful as the precondition-style APIs:

public static Duration ofMicros(long micros) { }
public static Duration ofSeconds(double seconds) { } // for legacy reasons, we have a *lot* of `double seconds` floating around inside of Google...yuck
public static Duration of(long value, TimeUnit timeUnit) { } // this is obsolete as of JDK9
public static long toMicros(Duration duration) { }
public static long toMicrosSaturated(Duration duration) { }
public static long toMillisSaturated(Duration duration) { }
public static long toNanosSaturated(Duration duration) { }
public static double toSecondsAsDouble(Duration duration) { }
public static DiscreteDomain<Duration> domain() { }

Makes Kotlin's extension-method feature look really appetizing, huh?

Also, minor thing, but we (Java Core Libs team) are generally -1 on the Utils suffix for classes. We probably could live with DurationUtils though if that's the consensus.

@jodastephen
Copy link
Member

So, Temporals already has 5 methods that operate on Duration. I think that is likely the best fit, even if we end up adding more.

@jodastephen
Copy link
Member

Actually, those 5 methods are on a branch (not master), so we do have naming room. Would you want another class named Durations though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants