On Thu, Dec 11, 2008 at 9:39 PM, Paul Lindner <plind...@hi5.com> wrote:
> Thanks for parsing the jslint output.
>
> FWIW this is the very same JS code that is running in most opensocial
> production instances today. To say it's not "production ready" just not
> true. The truth is that this code fails to pass an arbitrary set of
> guidelines. Try running PMD or findbugs on the Java code and you'll get a
> simliar list of scary errors that need to be fixed right now!
>
> Also, jslint is just stupid in some sections. For example:
>
>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:190:63:'escape'
>> is undefined.
>> return window.encodeURIComponent ? encodeURIComponent(str) : escape(str);
>>
>
>
> This is code that attempts to use encodeURIComponent on browsers that
> support it and falls back to escape() for ancient browsers. This is
> probably overkill since I doubt that OpenSocial performs very well on
> Netscape Navigator or IE4 :)
>
> In any case, I'll see about cleaning up some of these if it pushes things
> along.
>
> For the future it might be nice to enable the jslint maven plugin for the
> release target. That combined with standard criteria might serve us well
> for future revs.
We do have it enabled, but the default configuration is worthless, with
warnings like "only one var statement should be used per scope" polluting
any useful output.
>
>
>
> On Dec 11, 2008, at 8:41 PM, Henning P. Schmiedehausen wrote:
>
> As I mentioned before, I ran jslint in a very light setting on the
>> features javascript code. After applying the changes in SHINDIG-776 to
>> the tree, this leaves me with the following messages:
>>
>> a) eval is evil:
>>
>> While jslint flags these, I don't see a simple way to change
>> this. IMHO in these places, eval is probably the right answer. So no
>> reason to changes this unless someone comes up with a good solution to
>> this.
>>
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/
>> core.io/io.js:173:15:eval is evil.
>> var data = eval("(" + txt + ")");
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/auth.js:154:16:eval
>> is evil.
>> trusted = eval("(" + config.trustedJson + ")");
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/json.js:159:15:eval
>> is evil.
>> return eval('(' + text + ')');
>> ^
>>
>> b) undefined methods:
>>
>> I have no ideas where those methods are and what they do. Typos? Remnants?
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:190:63:'escape'
>> is undefined.
>> return window.encodeURIComponent ? encodeURIComponent(str) : escape(str);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:199:63:'unescape'
>> is undefined.
>> return window.decodeURIComponent ? decodeURIComponent(str) :
>> unescape(str);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/util.js:119:67:'unescape'
>> is undefined.
>> var unesc = window.decodeURIComponent ? decodeURIComponent : unescape;
>> ^
>> c) unclear breaks:
>>
>> Some of these seem to be intentional fallthroughs but this is sloppy
>> code at best. Either use if elseif else or document clearly what the
>> code does.
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:81:58:Expected
>> a 'break' statement before 'case'.
>> swfContainer = document.getElementById(swfContainer);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:85:6:Expected
>> a 'break' statement before 'default'.
>> }
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:92:21:Expected
>> a 'break' statement before 'case'.
>> opt_params = {};
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:155:22:Expected
>> a 'break' statement before 'case'.
>> flag = 1;
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:181:22:Expected
>> a 'break' statement before 'case'.
>> flag = 1;
>> ^
>>
>> d) scary for loops
>>
>> The explanation on the jslint homepage is sufficient enough for me
>> that I don't like these:
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:151:8:The
>> body of a for in should be wrapped in an if statement to filter unwanted
>> properties from the prototype.
>> for (var attrProp in attr) {
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-base/jsonactivity.js:75:2:The
>> body of a for in should be wrapped in an if statement to filter unwanted
>> properties from the prototype.
>> for (var field in oldObject) {
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/pubsub/pubsub-router.js:65:10:The
>> body of a for in should be wrapped in an if statement to filter unwanted
>> properties from the prototype.
>> for (var subscriber in channelSubscribers) {
>> ^
>>
>> f) C'tor names
>>
>> This is more coding style than anything else. However, most C'tors
>> adhere to this, only those fail. Yes, I understand that they probably
>> can't be fixed; however then this should be documented. Jslint is a
>> standard tool an people will run it on our code base, pointing these
>> things out again and again.
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-base/jsonactivity.js:67:26:A
>> constructor name should start with an uppercase letter.
>> fieldValue[i] = new className(fieldValue[i]);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-base/jsonperson.js:78:25:A
>> constructor name should start with an uppercase letter.
>> map[fieldName] = new className(fieldValue);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-base/jsonperson.js:86:26:A
>> constructor name should start with an uppercase letter.
>> fieldValue[i] = new className(fieldValue[i]);
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-reference/opensocial.js:458:23:A
>> constructor name should start with an uppercase letter.
>> this.prototype = new tempCtor();
>> ^
>> g) Line breaking errors
>>
>> Javascript is not Java. Especially when it comes to
>> whitespace. Applying Java linebreaking to Javascript code might break
>> it in subtle ways so I prefer not to do it unless necessary. While
>> SHINDIG-766 removes most of the line length motivated line breaks, the
>> ones on rpc.js between 288 and 349 remain, making the whole VB program
>> flag up in red. This should be an one-liner with explanations on top.
>>
>> h) Javascript URLs
>>
>> There is a lot of discussion whether one should use javascript:void(0)
>> or simply '#' as anchor. However, Jslint flags these.
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/tabs/tabs.js:430:17:Script
>> URL.
>> leftNav.href = 'javascript:void(0)';
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/tabs/tabs.js:451:18:Script
>> URL.
>> rightNav.href = 'javascript:void(0)';
>> ^
>>
>> i) functions in loops
>>
>> That code is a) unreadable and b) error prone. Needs fixing
>>
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:162:19:Be
>> careful when making functions within a loop. Consider putting the function
>> in a closure.
>> }).flag) {
>> ^
>> /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:171:15:Be
>> careful when making functions within a loop. Consider putting the function
>> in a closure.
>> }).join(arg));
>>
>>
>> IMHO, most of these things *should* be addressed before even thinking
>> to release this javascript code as "production ready". Either
>> documented as "this is intentional and a jslint exception around it"
>> or fixed. I prefer fixed.
>>
>> Ciao
>> Henning
>>
>> --
>> Henning P. Schmiedehausen - Palo Alto, California, U.S.A.
>> henn...@schmiedehausen.org "We're Germans and we use Unix.
>> henn...@apache.org That's a combination of two demographic
>> groups
>> known to have no sense of humour whatsoever."
>> -- Hanno Mueller, de.comp.os.unix.programming
>>
>
> Paul Lindner
> plind...@hi5.com
>
>
>
>
