Defensive coding
Hi there! How are you? I hope you are well!
Well, in the last few days I happened to bump into a very sloppy code in some part of the application I am working on daily: yes, we have code like this! Although, we are trying hard to maintain great quality and test our code it is very difficult to control every part of the app which is distributed among several teams. So, I got a call that we have a bug on that particular part developed by another team etc etc. Dear lord! It was a mess! I wonder how it worked to start with!!
So, here is a small function which wraps webview’s executeScript function:
executeScript(command, callback) {
this.win.executeScript({ code: command }, values => {
if (values && values[0]) {
callback(validateScriptResult(values[0]))
} else {
callback(values)
}
})
}
Well, here is the thing: if you read about the signature of the function you will see that the second argument is a callback and guess what! Optional! This is ok for the moment, because we wrapped it and we explicitly pass a callback to the this.win.executeScript (this.win is the webview). The problem arises on how we use the wrapped function: we either provide a command only or both command and callback arguments! We also have the callback argument optional! But, wait! We explicitly pass it down the internal function. If we do not pass a callback what happens?
Ops!!
Then again we have the internal callback with values etc… Where on Earth do we pass values there? I think it is the internal values passed there when the code is executed etc etc… So, we call that callback EVERY time… Aha!!
So, I got an error in the console: callback is not a function!
🙁
You know what this means at first glance? Yes, we pass a callback to the upper function which has a wrong type: it is either undefined or an array or I do not know what!!! Most probably undefined!
So, defensively thinking we do the following:
executeScript(command, callback) {
if (callback) {
this.win.executeScript({ code: command }, values => {
if (values && values[0]) {
callback(validateScriptResult(values[0]))
} else {
callback(values)
}
})
} else {
this.win.executeScript({ code: command })
}
}
Not very elegant maybe (some other dev proposed inline ternary on the argument etc.) but I really like it: it is super readable and conveys the message to a fellow developer of the callback being optional!
Another case
Lately, especially after TDD writing tests I found a lot of small uncaught things like the following.
In a React component we had a piece of code like this:
const { someProp } = this.props
and then after this,
if (someProp.property)
What is the problem with this check? It is very subtle to distinguish it at first sight!
The problem is that we are certain that we have an object there, which property or method we can freely call anytime we want! Boom! someProp can be undefined and guess what! It WAS in that particular case… for some inexplicable reason.
So, now every time I call a method in a check, I FIRST check if the object exists!! Simple as that….
if (someProp && someProp.property)
to avoid sky falling in our heads!!
Problems of this approach
When coding like this, we lose the information an exception can provide, e.g. for the first case we have a callback that is not working properly somewhere in the application and we know it because it raises the error. If we put the check there we get no info of this! On the other hand, if we had tested and coded that callback correctly we would not have crashed the system like this in the first place!
Like always, I think there is a tradeoff and a choice you need to make on how to handle errors and how to safeguard your code! A subtle bug now can break things (or maybe not) later on… But when it breaks them you need to make sure it does not halt the system to pause!
Until next time…
Cheers
PS: after I finished this post I searched for defensive programming! Well, what do you know! I am on point!
