Hi! In my opinion code review is not enough. It is important to have deep code review, but even promoting some king of defensive programming, it is difficult for a reviewer to take into account all possible cases (if we do not want code review becomes an endless process). So, besides code review we also need "testing", starting with unit testing with a good code coverage, and continuing with functional & non-functional testing with a good test case coverage.
Regards Marce El 05/10/2012, a las 21:45, James Lal escribió: 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]<mailto:[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]<mailto:[email protected]> https://lists.mozilla.org/**listinfo/dev-b2g<https://lists.mozilla.org/listinfo/dev-b2g> _______________________________________________ dev-b2g mailing list [email protected]<mailto:[email protected]> https://lists.mozilla.org/listinfo/dev-b2g ________________________________________________________ Marcelino Veiga Tuimil E-Mail: [email protected]<mailto:[email protected]> Telefónica Product Development and Innovation Tel: +34 91 483 27 38 Open Web Device Initiative C/Ronda de la comunicación, s/n, edificio oeste 1, planta 5 28050 Madrid (SPAIN) ________________________________ Este mensaje se dirige exclusivamente a su destinatario. Puede consultar nuestra política de envío y recepción de correo electrónico en el enlace situado más abajo. This message is intended exclusively for its addressee. We only send and receive email on the basis of the terms set out at: http://www.tid.es/ES/PAGINAS/disclaimer.aspx _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
