And don't calculate averages like Github Copilot or you'll get NaNs and/or panics when the denominator is zero
func averageRuntimeInSeconds(runs []Run) float64 {
var totalTime int
var failedRuns int
for _, run := range runs {
if run.Failed {
failedRuns++
} else {
totalTime += run.Time
}
}
averageRuntime := float64(totalTime) / float64(len(runs) - failedRuns) / 1000
return averageRuntime
}
NaN is not a bad result for something undefined like the average of an empty list.
I'd personally consider it bad code. It uses NaN as an error signalling vector, and makes it an unstated assumption. I suppose it would be acceptable if the project uses magic IEEE754 values / floating point traps this way, and everyone is aware of it.
I don't know Go, but from briefly skimming some articles, I believe a standard practice would be to define the function as:
and then detect the situation when all runs failed, and return an error. Alternatively, there should be a documented assumption that the function expects non-zero successful runs in its argument.
A sufficiently smart ML model could probably do either. This one doesn't, and the problem of NaNs is something I'd have a good chance of missing during code review.
In this case it's worse, because a divide by zero will panic instead of throwing an error.
The "standard" Go response is what you suggest. However, it does force the caller to deal with the error condition. For Go devs, this is standard procedure and they won't mind ;)
However, if the routine is relatively trivial, and it doesn't matter too much if an error occurs (but you don't want it to panic), then handling any errors inside the routine and always returning a valid result is OK.
If this was me, I'd take this second path, and keep the single return value, but catch the special case (float64(len(runs) - failedRuns) == 0) and return zero for that.
Or you could use the panic..recover method and trap the panic and return something sensible. I tend to avoid this, though, because it can trap panics further down the call chain (not in this example, obviously) and you end up with weird bugs that are hard to catch because they're not explicit.
Quietly returning something sensible is also a way to end up with weird bugs, except they are harder to find because it leaves open the possibility that the caller fails to check the return value for this token, and the program keeps humming along. At least log loudly so an attentive developer has a chance of finding the bug. I guess this is now an an old-school practice, but I've always been a believer in throwing an exception on error so that they are totally obvious and you don't ship with them. Crash early, crash often. Find the bug before it hits production.
yeah, always use a static analysis tool to check for unchecked error values. Saved my arse so many times!