On Wed, Nov 16, 2011 at 3:15 AM, Owen Davis <[email protected]> wrote:
> John Du Hart <compwhizii <at> gmail.com> writes:
>
> > It's just another style I've encountered on other projects and I
> > personally like.
>
> The syntax itself is fine, but at Wikia we have found (after a recent post
> mortem) that out of 23 "Fatal Error" code defects found in production,
> 7 of them were due to method calls on null object references. If any
> method in that chain returns null then the request fails. It most cases
> core MediaWiki objects do return a valid stub object of some kind, but
> not all of them do (and in some cases they return null. intermittently
> so the code works "most of the time", which is in many ways the worse
> scenario). Introducing a pattern like this in a code base this large is
> therefore problematic. The "clean looking code" benefit is perhaps
> outweighed by the fact that you need to add extra "if" conditions or
> try/catch blocks everywhere to handle local null object exceptions.
> In our case, we are trying to ensure during code reviews that we just
> check for null objects in all "if" conditions, which does also lead to more
> highly nested and less readable code.
>
Any individual interface should generally have one of three patterns:
A) cannot fail, ever -- always returns a live object
B) always returns a live object -OR- throws an exception
C) returns a live object -OR- null/false as error
The most annoying problems seem to come from treating C code as if it's A:
everything works fine until you come across some invalid input, and then
somewhere else in your code you die, utterly surprised.
The thing I like about B) is that failing to handle the error case gives
more immediate feedback: the error output & backtrace tell you where the
*thing that didn't find its data* happened, instead of where some other
code tried to use that previously-fetched data.
My ideal unicorn-magic world tends to fall mostly in the A-B spectrum
(though exceptions do not always make sense in every case.).
Throwing chaining onto things like OutputPage could be done but seem
unlikely to be worthwhile; the operations are generally independent of each
other.
Chaining is good for things that work conceptually like a filter chain. For
instance in jQuery code we'll often do something like:
$(element).find('a')
.click(magicClickHandler)
.attr('title', 'Click for magic!');
The real magic actually came from the progress from $(element) to
.find('a') and then to performing operations on the resulting collection.
There's a much smaller difference when there's a single variable as a
common prefix:
$foo
.click(magicClickHandler)
.attr('title', 'Click for magic!');
vs
$foo.click(magicClickHandler)
$foo.attr('title', 'Click for magic!');
is only slightly "cleaner".
> Everything is a tradeoff, but IMHO you do need to check for null in chains
> like that, which means you can't really get away with it (at least not for
> long).
>
jQuery's chaining API is explicitly built to make sure that you don't --
it's style A in the layout above. Each step in the chain returns a
collection, even if it's an empty collection... acting on an empty
collection may not do anything, but it doesn't cause a fatal error!
Any chaining APIs we do add should similarly be built explicitly this way,
such as the Message class which we have both PHP and JavaScript versions of.
-- brion
_______________________________________________
Wikitech-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
