This makes quite a bit of sense to me, particularly when dealing with anything that relates to the users money. I would also make the point that units tests to vet he positive and negative tests around this condition would likely have prevented this problem.
- James On Fri, Oct 5, 2012 at 12:26 PM, Nicolas B. Pierron < [email protected]> wrote: > 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<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<https://lists.mozilla.org/listinfo/dev-b2g> > _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
