Hi,
A bit of context before starting arguing. I am the lucky winner of
dogfooding lottery (see Bug 797232 [1]) and hopefully the last one. I was
quite surprised by the final fix which is just a missing return statement.
The missing return statement was far from the “this.sms.send” statement, and
when I see that the cost control application was landed in one pull-request
containing 4,639 added lines, I wonder how a code review can be effective.
What I am suggesting, is to adopt a coding convention, or better an API
change to attract the attention of reviewers and enforce developer to
develop in a defensive way.
The principle is simple, every time you can induce cost to the user, you
ensure that you have a “Go” before continuing. Even if you are convinced
that it will never happen, you add a guard to handle any unforeseen reason
which might cause this test to be wrong.
// Coding convention
if (everythingOk()) {
this.sens.sms(…);
} else {
warn("The impossible just happened!");
}
or
// API enforced coding convention
this.send.sms(everythingOk, …);
There are multiple advantages at doing so:
* This won't cost money if there is something wrong.
* This pin-point the functions which have to be reviewed carefully.
* This would be just a normal issue and not a blocker one.
Defining this as part of the API will enforce developer to provide such a
function, and thus brings attention on the defensive programming aspect.
What do you think?
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=797232
--
Nicolas B. Pierron
_______________________________________________
dev-b2g mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-b2g
