Hacker News Reader

Comment by contravariant | Hacker News Reader

Well ignoring the silly stuff this would just boil down to something like:

    request = NetHttpTransport.Request()
    request.setConnectTimeout(getDefaultTimeoutFromEnvironment().seconds)

which is verbose, but at least it's fairly clear.

And yes I'm calling a class named HttpRequestInitializer silly, I don't care if some language decided it should exist.

View on HN

    request.setConnectTimeout(getDefaultTimeoutFromEnvironment().seconds)

This is actually a very good example of what not to do, with the mistake being in whoever implemented setConnectTimeout()

I actually don't know this particular API, but I'm used to timeouts being in milliseconds, so that code looks wrong to me.

Much better API, and what the article is talking about, is to change this to:

    .setConnectTimeoutMilliseconds(getDefaultTimeoutFromEnvironment().seconds)

Now the mistake is obvious, and even if the original developer doesn't notice it will stick out in a PR or even a causal glance.

This example is nice but if you put arguments metadata in the function name, you have to have one main argument, the function name can prove cumbersome if you have 3 or 4 arguments with units like

    .setPricePerMassInCentsPerKilogramsWithTimeoutInMilliSeconds(100,2,300)

I think you should rather do

  .setPrice(priceInCents=100, massInKg=2, timeoutInMs=300)

I'd argue there are better API patterns for this though -- keeping in mind this values code readability (and correctness) over micro-optimization:

    .setPriceInCents(100);
    .setMassInKilograms(2);
    .setTimeoutInMilliseconds(300);

    .calculate({
        priceInCents = 100,
        massInKilograms: 2,
        timeoutInMilliseconds: 300,
    });

Values with an intrinsic unit scale up to many units and values. If you declare the parameters of a "calculate" function as USACurrency, Weight and Duration you can write

    calculate(100cent, 2Kg, 300s)   
    calculate(0.1dollar,4.7lb/*approximate*/,5min)
    calculate(something.price(),whatever.weight(),options.getDuration("exampleTimeout"))
    calculate(USD(0.1),Kg(2),Minute(5))

That's one of the benefits of having it exposed as a type-enforced parameter. setConnectTimeout() could take a Duration, which contains the amount and the unit, and therefore wouldn't care if consumer A provided a timeout in seconds, and consumer B provided a timeout in milliseconds.

Totally agree, but then I would expect the code would be:

    request.setConnectTimeout(getDefaultTimeoutFromEnvironment())

with getDefaultTimeoutFromEnvironment() returning a Duration.

Ideally this is consistent throughout the codebase, so that anything that uses a primitive type for time is explicitly labelled, and anything using a Duration can just be called "Timeout" or whatever.

All fair points. In this example I was suggesting what this might look like at the border of the application where you need to talk to some (standard) library which doesn't use the same convention.

A class named 'HttpRequestInitializer' and taking 10 lines to set a timeout on a HTTP request isn't merely hypothetical: https://developers.google.com/api-client-library/java/google... - and that's not counting any import statements.

(although getDefaultTimeoutFromEnvironment was artistic license on my part)

True, but like I said that doesn't make it not silly. Just harder to fix.

Edit: Also, overriding a class method dynamically inside a function? I usually program python these days and even I think that's wild.

Its an interface with only method. I suspect that most of us would just us a lambda now.

I'll never quite understand why it wasn't simply a function in the first place.

Java pre-8 only had anonymous classes, there were no lambdas

And Java lambdas are still syntax sugar for one-method anonymous classes.

This is where I divert. You just hard coded seconds into your test. Now your tests that cover this must take seconds to finish; thankfully you were not testing hours or days!

My last shop was a Go shop and one test I think shows this off was an SMTP server and we needed to test the timeouts at different states. The test spun up half a dozen instances of the server, got each into the right smtp state and verified timeouts in under 10ms.

The environment that set the timeout would either be "timeout_ms=500" or "timeout=500ms" (or whatever). This is where you handle that :)

Many timeout functions take seconds as a floating point. So you could time out on 0.05 seconds (5 milis). But now the code is clear and less prone to bugs.

In Go, you don't pass a float, you pass a duration

Which is unitless, hence there is no problem. `time.Duration.Seconds()` returns a floating point.

Not sure I fully understood your objection, but the reason I specified seconds is because I presumed the setConnectTimeout to be part of the default HTTP library, which likely doesn't adhere to the same conventions, and that it expected seconds (which seem to be the usual for http libraries as far as I can tell).

Of course if the setConnectTimeout method was part of the same application you could just pass the timeout directly, but at the boundary of your application you're still going to have to specify at some point which unit you want.

The test could simply mock the default value to something reasonable like '.1 seconds' and test that duration instead, so I don't think this is a real problem.

If you're testing things with timeouts it's often a good practice to mock out the system clock anyway. That allows testing your timeouts to be nearly instantaneous and also catches edge cases like the clock not moving forward, having a low resolution, moving backward, ... deterministically.

This is actually revealing a different problem: the system clock as an implicit dependency. YMMV depending on support of underlying libraries, but I will typically make the clock an explicit dependency of a struct/function. In Go, usually it’s a struct field `now func() time.Time` with a default value of `time.Now`.

Reply on news.ycombinator.com