On Mon, Oct 7, 2013 at 2:56 PM, Brion Vibber <bvib...@wikimedia.org> wrote: > > "We already have three major error-reporting patterns: return null/false, > throw exceptions, and return a Status object. Can you justify the idea of > adding a fourth with strong benefits compared to the existing three?" > > -- brion
Exceptions: After re-reading my initial post i can see where some confusion might come in, I do not advocate banning exceptions, rather I advocate using exceptions only for exceptional circumstances. What constitutes an exceptional circumstance can vary from developer to developer, but if we agree that exceptions are only for exceptional circumstances then we agree exceptions are only a piece of the solution. In addition to the above there is the question of using exceptions for control flow. At a past job our lead(who came from enterprise java) used to say "If the language wanted to give you a statically typed stack breaking return statement they would give you one. Don't hack it on top of Exceptions." I still agree with this, but as with what is an exceptional circumstance intelligent developers are allowed to disagree here. return null/false: If you buy into the notion that exceptions should be used for exceptional circumstances then you need something to return in those not so exceptional circumstances. The go to value's within php are either null or false depending on context. Within Flow we have both exceptions and null/false return values. As lead developer on the Flow project I feel it is my responsibility to recognize and attempt to find solutions to recurring problems within the development team. We are semi-regularly finding newly submitted code that fails to handle the null/false case originating from both core and our own code. This is not a new problem unique to Flow, one of our developers mentioned that the reason he checks for them so much in reviews is that he had the same issue in his very first WMF patch. I think developers time could be better used than looking up all the function calls used in a patch to see how they handle errors. I'm not trying to and don't expect anything to change in core, but I wanted to bring in battle tested code used by thousands of other developers to solve development problems within the extension we are writing. Status class: A review of the code within this class tells me that its primary use case(its single responsibility if you will) is to assist the caller in generating user facing error messages about an operation that can fail. If null/false is an acceptable return value then there is no reason to use this class. If this were a smaller codebase and php-option was an accepted solution I would investigate(e.g. review all the places its used and see if its even solving the same problems) replacing the Status class usage with an extended version of php-option's None class that assists the caller in generating user facing error messages. But this is not a small codebase. This is a 10 year old codebase with over a million lines of code and many baked in assumptions. It would be, at best, ridiculous to go around breaking code that currently works. So, then what? As stated above we have a problem within Flow development. Developers are not consistently checking every possible source of null/false. We could just call it a developer error and tell us all to smarten up. But I think we have the opportunity to do better. A better error handling solution should be obvious when doing code review. A reviewer should be able to know if the error conditions are properly handled by looking at the new code, not by looking up all the function calls to see what they can possibly return. I am not particularly tied to php-option. I do have a problem that I need to solve though. I have hopefully addressed why the existing error handling solutions within mediawiki are not solving this problem. I am assuming that if some null/false errors are regularly being caught in code review then others are sneaking through into master. I don't know how to solve these problems using the solutions currently in mediawiki so I looked to what is being done in the wider developer audience. Moving Forward I honestly don't know what to do moving forward. So far the plan is keep using null/false, and hope that we don't ruin too many wikipedian's day when some function has a non-exceptional error condition and blows up the extension. Erik B. _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
