Right now our coding conventions manual never touches on method chaining, nor have I personally seen this practice in core. So I'm interested in what the rest of the community feels about adapting this practice more, and if there are trade offs I'm not aware of. Let's make an example, take this code from Abuse Filter:
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
So, instead of writing it like that, it could be written
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
It's just another style I've encountered on other projects and I personally like.
On 16 November 2011 06:50, John Du Hart compwhizii@gmail.com wrote:
Right now our coding conventions manual never touches on method chaining, nor have I personally seen this practice in core. So I'm interested in what the rest of the community feels about adapting this practice more, and if there are trade offs I'm not aware of.
I believe I introduced it into the core first time with the Message class. There were some resistance and concerns, but it looks like there hasn't been any problems using it.
-Niklas
Actually, I love the idea. Turbo Pascal's Turbo Vision framework was heavy into this. Although it did tend to get a little crazy trying to match up parens when you started to define something like a really long menu, for example.
Actually, isn't there an XML class doing this in a few places?
Dan
On Tue, Nov 15, 2011 at 11:50 PM, John Du Hart compwhizii@gmail.com wrote:
Right now our coding conventions manual never touches on method chaining, nor have I personally seen this practice in core. So I'm interested in what the rest of the community feels about adapting this practice more, and if there are trade offs I'm not aware of. Let's make an example, take this code from Abuse Filter:
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
So, instead of writing it like that, it could be written
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
It's just another style I've encountered on other projects and I personally like.
-- John Du Hart _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Tue, Nov 15, 2011 at 11:50 PM, John Du Hart compwhizii@gmail.com wrote:
Right now our coding conventions manual never touches on method chaining, nor have I personally seen this practice in core. So I'm interested in
what
the rest of the community feels about adapting this practice more, and if there are trade offs I'm not aware of. Let's make an example, take this code from Abuse Filter:
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
So, instead of writing it like that, it could be written
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
It's just another style I've encountered on other projects and I
personally
like.
-- John Du Hart
All our (newer) JavaScript is full of this sort of syntax, although it does somehow look messier with the right-arrow than the period in jQuery. I don't see a problem with using the technique in our PHP code as well.
--HM
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.
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).
Hey,
Introducing a pattern like this in a code base this large is therefore
problematic.
I'm tempted to agree with this. Doing
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
is less clear then doing
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
Since it's not obvious if addWikiMsg is getting called on the output object or on something setPageTitle returns in case of the former. It'll also break if someone adds a return value.
IMO this is asking for bugs.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Wed, Nov 16, 2011 at 12:36 PM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
Introducing a pattern like this in a code base this large is therefore
problematic.
I'm tempted to agree with this. Doing
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
is less clear then doing
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
Since it's not obvious if addWikiMsg is getting called on the output object or on something setPageTitle returns in case of the former. It'll also break if someone adds a return value.
IMO this is asking for bugs.
I fully agree.
Bryan
On Wed, Nov 16, 2011 at 12:36 PM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
Introducing a pattern like this in a code base this large is therefore
problematic.
I'm tempted to agree with this. Doing
I think chaining is great for classes where 1) it makes sense and 2) the class is designed for it from the get-go. The Message class fits this, but I don't think OutputPage does.
Roan
On Wed, Nov 16, 2011 at 6:49 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
I think chaining is great for classes where 1) it makes sense and 2) the class is designed for it from the get-go. The Message class fits this, but I don't think OutputPage does.
Ya, something from the ground up definitely seems like the place where this is practical (ie: turbo vision). I guess this doesn't work so well when all the code is bootstrapped up and pieced together slowly over years, from hundreds of devs with their own coding styles/preferences/conventions/etc. Yay open source ;)
(...)
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
vs
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
And if $out was indeed null, just the line number would point out which object was, instead of needing to test the whole chain.
Call chaining works really well for something like jQuery where you are constantly working with a generic type of data (a selection of dom nodes in that case) with common methods. Most of what you do in jQuery is either modifying what is selected, or calling methods on the selection.
There are some "getter" versions of various methods that naturally don't chain, but all other methods that can be called on a selection are chainable. It's this momentum in the library that makes the practice easy for developers, because it's easy to learn the few exceptions when its 90% chainable vs. 10% not-chainable. I have doubts that enough of MediaWiki's data structures would be conducive to chaining for reasonable momentum to be established.
Another issue is, until we have PHP 5.3 as a base requirement, the use of closures isn't allowed in MediaWiki. Closures are not required for call chaining, but they round out the solution by allowing arbitrary bits of code to be passed around through callbacks and iterators, making call chaining a more viable alternative for most tasks.
To the other points, about null results being treated as objects - this is all part of why call chaining is something that should be a part of the the overall architecture and not something you slap on at the last minute like a bumper sticker.
PHP is a rather primitive language, and call chaining is a fairly advanced technique (when done right) - it's really not a very good match.
- Trevor
On Wed, Nov 16, 2011 at 10:02 AM, Platonides Platonides@gmail.com wrote:
(...)
$this->getOutput() ->setPageTitle( wfMsg( 'abusefilter-examine' ) ) ->addWikiMsg( 'abusefilter-examine-intro' );
vs
$out = $this->getOutput(); $out->setPageTitle( wfMsg( 'abusefilter-examine' ) ); $out->addWikiMsg( 'abusefilter-examine-intro' );
And if $out was indeed null, just the line number would point out which object was, instead of needing to test the whole chain.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Nov 16, 2011 at 3:15 AM, Owen Davis owen@wikia-inc.com 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
What problem is solved by chaining? All I see here is cost for no benefit. On Nov 16, 2011 5:02 PM, "Brion Vibber" brion@pobox.com wrote:
On Wed, Nov 16, 2011 at 3:15 AM, Owen Davis owen@wikia-inc.com 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
In jQuery the benefit is less code do do the same thing, which means less download time for the same functionality. This obviously doesn't apply to PHP.
There may be other benefits to chaining in PHP code, but I don't really know of them.
- Trevor
On Wed, Nov 16, 2011 at 3:01 PM, Russell Nelson russnelson@gmail.comwrote:
What problem is solved by chaining? All I see here is cost for no benefit. On Nov 16, 2011 5:02 PM, "Brion Vibber" brion@pobox.com wrote:
On Wed, Nov 16, 2011 at 3:15 AM, Owen Davis owen@wikia-inc.com 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
wikitech-l@lists.wikimedia.org