We have recently introduced two defects, while trying to improve old, obscure code. Of course we are to blame for not being more paranoid and not researching more why the code was there. But if the code was clear to start with, if it followed the least astonishment principle, the errors wouldn’t have happened. In both cases the original code provoked the “WTF?!” reaction in the reader.
Case 1: Ignore optional property when missing
We had a React component with two ways to control it being shown or not: either by setting showInitially or by setting show. This piece of code was intended to handle the latter case:
onSetProperties(props) {
if (!props.show) return;
this.setState({ show: props.show })
}Why the if was necessary was quite unclear.
This would have been much more explicit and thus better:
onSetProperties(props) {
if (_.isUndefined(props.show)) return;
this.setState({ show: props.show })
}Even better, (which we eventually did), the component would be refactored to support just a single way of being displayed/hidden, that fit all use cases.
Case 2: Returning an “error data” when resource not found
We had this function:
function getUserAccountData(userId) {
return httpRequest(`/service/account/${userId}`).then(response => {
if ([200, 404].includes(response.httpStatus)) return JSON.parse(response.body);
else throw new RequestFailedError(response);
})
}So, when the user did not exist, it returned something like this:
{
error_code: 20
reference: '5ee7c248-6865-4458-d06a-21e34c10456d'
}instead of the normal account data. That was contrary to all the other services, that threw either the generic RequestFailedError or, where needed, the more specific NotFoundError. And it made it more difficult for a new usage that actually needed to treat 404 as an error.
So, not to surprise the reader, we could do one of these upon 404:
1. Return null and, preferably, rename the function to indicate that it can return that (contrary to most other ones) to e.g. getUserAccountDataIfExists.
2. Throw NotFoundError and let the piece of code that accepts 404 to catch it and turn into null or whatever it needs.
Conclusion
Try to be explicit and as clear as possible, follow the least astonishment principle – i.e. make sure that your code behaves in the same way as similar code elsewhere in the app.
| Published on Web Code Geeks with permission by Jakub Holy, partner at our WCG program. See the original article here: WTF code, defects, and the principle of least astonishment Opinions expressed by Web Code Geeks contributors are their own. |
