A team was bringing forward an API for review, and one of the parts involved expressing a set of supported values in the form of three numbers:
- A minimum allowed value.
- An increment.
- A maximum allowed value.
The supported values are the minimum, integer multiples of the increment added to the minimum, up to and including the maximum.
For example, if the minimum is 5, the increment is 10, and the maximum is 30, then the valid values are
5 | Minimum |
15 | Minimum + 1 × Increment |
25 | Minimum + 2 × Increment |
30 | Maximum |
The team also noted that if the increment is zero, then the supported values are only the minimum and maximum.
I pointed out that this design tempts the user to divide by zero.
Here’s a function you might write:
int closestSupportedValue(int desired) { int nearest = minimum + ((desired - minimum + increment/2) / increment) * increment; return std::clamp(nearest, minimum, maximum); }
We first find the multiple of the increment beyond the minimum that is closest to our desired value. (You can adjust the rounding by changing the +increment/2
appropriately.) We then clamp that value to the allowable range.
This works great unless the increment is zero. And it’s possible that the developer never encounters a zero in all of their testing, say, because all of the hardware they tested their code on support a wide range of possible values. They never realized that they had to test on hardware that supported only 1 or 2 possible values.
I suggested that the team remove the zero from the API. If only two values are supported, then set the increment equal to maximum - minimum
.¹ If only one value is supported, then set the increment to 1.
Bonus chatter: The power grid team fell into this trap. The PowerGridForecast
class represents a series of PowerGridData
objects, starting at the StartTime
, where each prediction represents a length of time described by the BlockDuration
. In other words, the nth element of the vector represents a prediction that starts at StartTime
+ n × BlockDuration
and lasts for BlockDuration
.
To write a function that finds the block that describes a specific point in time, you might come up with something like this:
PowerGridData FindForecastForDateTime( PowerGridForecast forecast, DateTime time) { var elapsed = time - forecast.StartTime; var index = elapsed / forecast.BlockDuration; if (index < 0 || index >= forecast.Forecast.Count) { return null; // no forecast for this index } return forecast.Forecast[index]; }
We determine the elapsed time since the start of the first forecast, determine which block index it belongs to, and return the element at that index, if it exists.
The team decided that if there was no forecast, then the Forecast
property would return an empty vector (good), and the BlockDuration
would be zero (bad).
The team figured that if there is no forecast, the block size is irrelevant since it’s describing something that doesn’t exist, so who cares if it’s zero? But we saw above that a zero block duration means that the calculation of the index performs a divide-by-zero.
Since the block size is irrelevant, they should have chosen a value that is unlikely to cause problems. Pick a nonzero block size that is typical of a valid block size, like say, one hour. In practice, most developers people are testing on machines with good Internet connectivity and which are physically located in areas that have good power grid forecast data. They don’t have test machines in a remote country.²
¹ Or really, you could set the increment to any positive value larger than maximum - minimum
, though you don’t want to choose an excessively large value, because that risks integer overflow.
² And they are unlikely to have the budget to fly a developer to a faraway country just to ensure that this test case passes. “Boss, it’s important that we have a developer in Tahiti, to ensure we have proper test coverage.”
oh this gives me nightmares and flashbacks about horribly designed scrollbars/slider, that have minimum/maximum properties and you always have to write a complicated algorithm when settings those by testing currently set values otherwise, "maximum must be larger than minimum", "value must be between min and max".... so you have to calculate which of the 3 to set first, second and last.
and on another note: I've seen developers "choose" to switch to floating point to avoid "division by zero" but then run into trouble and weird behavior many code lines later. Comparisons work fine with just one infinity, (to check like...
My first response to the spec before "For example" is that max need not be a candidate value, because (max - mini) need not be a multiple of step, e.g., mini = 0, max = 11, step = 5, then you can choose 0, 5, or 10. (The phrase "up to and including" means "up to and not excluding" in my book.) Then
>The team also noted that if the increment is zero, then the supported values are only the minimum and maximum.
This note is so illogical. If step is 0, the only supported value is of course the minimum. (I'm...