PDA

View Full Version : [UNKNOWN]ExtJS source lots of "inconsistent return points"



stephen.friedrich
7 Oct 2009, 11:03 AM
I was browsing ExtJS source code with IDEA and it showed me a couple of error messages.
So: Is it intentional that many functions whose docs specify that they return "this", in fact return undefined in some branches?

See for example Ext.BoxComponent.setPagePosition() (see screenshot).

MiamiCoder
8 Oct 2009, 11:12 AM
I would not think this is intentional. This is probably something the dev team needs to take a look at.

Animal
8 Oct 2009, 1:21 PM
Create a bug thread. That's a bug.

stephen.friedrich
8 Oct 2009, 4:39 PM
Give me a couple of days, then I can let IDEA run over the current trunk, fix the warnings and submit a patch.
I am always too busy on workdays.

MaxT
9 Oct 2009, 12:55 AM
In the example above I would say the docs are wrong. You need to be able to give some indication of whether the method method worked or not. If you just return "this" you have no idea if it worked correctly.

Having said that there are many instances of not returning values consistently. (Just turn on strict javascript warnings to see this.) My understanding is that this is to save characters, at the slight expense of making the methods consistent with strict coding standards.

Max

stephen.friedrich
9 Oct 2009, 1:16 AM
I see, that makes sense. In that case the docs should really be updated for example

@return {Ext.BoxComponent} this if the position was changed,
undefined if the position was not changed (because !this.boxReady or the input x or y was undefined)

Animal
9 Oct 2009, 3:37 AM
The docs are correct. The code is wrong. It should return itself to allow multiple calls to Component methods in one statement.

stephen.friedrich
9 Oct 2009, 9:51 AM
I had a look at all these warnings. However I think you really can't change all the issues blindly by returning the appropriate type.

For example a tree node returns false from appendChild() if a 'beforeappend' or 'beforemove' event listener returned false. I can't tell if that is used anywhere.

So probably it's better to leave it at the current state until somebody has a real use case where the current code causes a bug.

Or else carefully review all the warnings and make a case-to-case decision to change either the docs or the code - but check if some client code relies on the current state of the code before the code is really changed.

Here's a list of the warnings:

Tree.js, #363, appendChild
Element.scroll-more.js, #79, scroll
DomHelper.js, #223, insertIntoTable
Ext.js, #421, each
BoxComponent.js, #438, setPagePosition
Container.js, #755, doLayout
MessageBox.js, #408, setIcon
Panel.js, #1341, collapse
Panel.js, #1378, expand
Provider.js, #55, decodeValue