Forum
Log In
Home
Topic RSS
00:40
10/08/2009
Offline
Hello Tony
short time before I fix some bugs in my own javascripts with respect of JSLint tool (http://www.jslint.com/lint.html). This tool is written by Douglas Crockford, the author of “The Good Parts” book and founder of JSON. How one can see in http://github.com/jquery/jquer.....ter/build/ John Resig uses JSLint also during development of jQuery. I am not fan of Crockford, but he is a greatest specialist in JavaScript and his tool could really help to makes javascript code better.
For example, If one try analyze only one grid.base.js file, one could find following
Line 52
xmlDoc["loadXM"+"L"](xmlString);
can be changed to
xmlDoc.loadXML(xmlString);
Line 336
if (p.gridComplete = complete) {
Can be fixed
if (p.gridComplete === complete) {
On can also find two places (line 614 and line 1905) where missed “var” declaration:
len = drows.length, i=0;
and
data = rdata[k];
which seems should be fixed to
var len = drows.length, i=0;
and
var data = rdata[k];
The line 1406
if (!ts.p.viewsortcols[2]) { r=true,d=t.attr("sort") }
Should be probably fixed to
if (!ts.p.viewsortcols[2]) { r=true;d=t.attr("sort"); }
The lines 1896-1897
if (!ts.p.viewsortcols[2]) { r=true;d=t.attr("sort"); }
cmn = t.p.colModel[t.p.keyIndex+gi+si+ni].name;
if(typeof rdata[0][cmn] != "undefined") rowid = rdata[0][cmn];
should be fixed to
cnm = t.p.colModel[t.p.keyIndex+gi+si+ni].name;
if(typeof rdata[0][cnm] != "undefined") rowid = rdata[0][cnm];
because in the line 1876 is defined variable cnm (and used later in the line 1908) and not cmn.
JSLint tool could help to find some missing semicolon. For example, line 339 or line 384
result += "width: "+grid.headers[pos].width+"px;"
(which should be fixed to "result += "width: "+grid.headers[pos].width+"px;";") . See also lines 441, 696, 788, 833, 1182, 1402, 1643, 1668 for the same problems.
Multiple definitions of variables (compare line 614 with 592 in the same scope) could be also detected.
Some places where expected an assignment or function call and instead saw an expression: Lines 555, 661, 696, 1370, 1947, 2304.
Some unimportant places like line 1851 with double “;” and the end of statement are also nice to find.
Unescaped '-' in regex in the lines 961 and 962.
Recommendation to replace line 1982
function isEmpty(obj) { for(var i in obj) { return false; } return true; }
with a code like
function isEmpty(obj) {
for(var i in obj) {
if (obj.hasOwnProperty(i)) { return false; }
}
return true;
}
to filter unwanted properties from the prototype.
Suggestion to replace Function constructor line 196
document.onselectstart=new Function ("return false");
with a standard function() {…} construction. (The same problem exists in the line 252.)
Line break errors: for example lines 61-63
msg = ($.jgrid.useJSON===true && typeof (JSON) === 'object' && typeof (JSON.parse) === 'function')
? JSON.parse(js)
: eval('(' + js + ')');
One should be replaced these lines to
msg = ($.jgrid.useJSON===true && typeof (JSON) === 'object' && typeof (JSON.parse) === 'function') ?
JSON.parse(js) :
eval('(' + js + ')');
Very strange places commented as “Expected an assignment or function call and instead saw an expression”: lines 555, 661, 2303-2304 (better to use if-statement), 1370 and 1947.
One can continue these suggestions.
So for me I decide to use JSLint tool in my project. Because I develop in Visual Studio, I use now mostly JSLint.VS (http://jslint.codeplex.com/). But I think also use the original fulljslint.js with a way like described in http://jason.diamond.name/webl.....al-studio/ (or some modification of this way). Another possible way is to use http://www.javascriptlint.com/). Other Plugin jslint.vim (http://github.com/hallettj/jslint.vim) can be also interesting.
I recommend you Tony to invest a little time in JSLint tool. It will help to make the code of jqGrid better.
Best regards
Oleg
11:48
Moderators
30/10/2007
Offline
Hello Oleg,
First of all thank you for this.
We have missed this checking since 3.5 version.
The most of your suggestions are allready done. Will be published soon.
Just one note
Line 336
if (p.gridComplete = complete) {
this is a valid instead and can be rewritten like this
p.gridComplete = complete;
if(p.gridComplete) {
It is written so to save the place 
your fix will break the code.
Again thanks
Best Regards
Tony
For professional UI suites for Java Script and PHP visit us at our commercial products site - guriddo.net - by the very same guys that created jqGrid.
12:55
10/08/2009
Offline
Hello Tony,
with the line "if (p.gridComplete = complete) {" I was not sure myself.
If you want I could check other jqGrid-files (not only grid.base.js) when I found a little time for it.
Regards
Oleg
15:44
10/08/2009
Offline
Hello Tony,
now some suggestions to improve grid.common.js:
Replace the line 234
var curleft = curtop = 0;
with the line
var curleft = 0, curtop = 0;
Line 254
if(options.defaultValue) delete options['defaultValue'];
with the line
if(options.defaultValue) delete options.defaultValue;
the lines 260-261
delete opt['id'];
delete opt['dataInit'];
with the lines
delete opt.id;
delete opt.dataInit;
and the line 270
delete opt['dataEvents'];
with the line
delete opt.dataEvents;
the line 308
try {delete options['value'];} catch (e){}
to the line
try {delete options.value;} catch (e){}
the line 327
try {delete options['dataUrl'];delete options['value'];} catch (e){}
to the line
try {delete options.dataUrl;delete options.value;} catch (e){}
the line 332
delete options['buildSelect'];
to
delete options.buildSelect;
In the line 341 are missing semicolon. One should replace
ovm = jQuery.map(ovm,function(n){return jQuery.trim(n)});
to
ovm = jQuery.map(ovm,function(n){return jQuery.trim(n);});
Because the same reason one should replace
ovm = jQuery.map(ovm,function(n){return jQuery.trim(n)});
to
ovm = jQuery.map(ovm,function(n){return jQuery.trim(n);});
line 374. Don't make functions within a loop.
In the line 386 are used variable ov
ov = document.createElement("option");
which is defined out of scope (see line 370). So it would be better insert for example in the line 360
var i;
definition of ov variable
var i, ov;
and remove it from the line 370
var oSv = options.value, ov;
Lines 386-391 (for-body) should be placed inside of the body of the following if-block
If (oSv.hasOwnProperty(key)) { }
to filter unwanted properties from the prototype.
The line 446
for( i =0, len=g.p.colModel.length;i<len; i++){
has len variable, which is not defined. So one could modify the line 444
var edtrul,i, nm;
to
var edtrul,i, nm, len;
In the line 546-547 are missing semicolon.
if(yln.indexOf("m") != -1) {mln=i}
if(yln.indexOf("d") != -1) {dln=i}
So one should replace these lines to the following
if(yln.indexOf("m") != -1) {mln=I;}
if(yln.indexOf("d") != -1) {dln=I;}
Moreover in the line 374
sv[1] = jQuery.map(sv,function(n,i){if(i>0){return n;}}).join(":")
the function “function(n,i)” is defined within a loop (see line 371). If would be better to define it outside the loop and use it only in the line 371. The corresponding change could be following. We insert a new line which looks like
var myMapFunc = function(n,i){if(i>0){return n;}};
(please, verify what should this function returns if i=0) after the line 370 and change line 374 to
sv[1] = jQuery.map(sv, myMapFunc).join(":");
Best regards
Oleg
19:53
10/08/2009
Offline
Hello Tony,
I add here some more LS Lint suggestions which I forgot to include in my first post.
In the file grid.base.js in the line 713 are used function IntNum which should be better renamed to intNum. The function with the first capital letter in the name looks like more as a name of a class.
Assignment in the line 967 looks a little strange because of "+=":
h = (h !== 12) ? h += 12 : h;
The Line 2304 looks like also strange because of assignment "=" inside:
mathopr ? sum += parseFloat(val) :
obj ? ret.push({id:$t.rows[i].id,value:val}) : ret[i]=val;
Variable bs declared in the first line of the function updatepager (the line 689) is never used.
Variable imgs declared in the line 1117 is never used.
Variable ht declared and assigned in the line 1364 is never used later.
Variable rowslen declared and assigned in the function (the line 1887) is never used later. Instead of rowslen variable t.rows.length is used in the lines 1910, 1935, 1940, 1958 and two times in the line 1966.
Variable ind declared in the line 1987 is never used.
Variable tn declared and assigned in the line 2139 is never used later.
Best regards
Oleg
18:01
Moderators
30/10/2007
Offline
Hello Oleg,
Thanks again for this. I will use this link to do the changes.
Of course we need to make these very carefully and apply them to the next jqGrid release.
Best Regards
Tony
For professional UI suites for Java Script and PHP visit us at our commercial products site - guriddo.net - by the very same guys that created jqGrid.
14:29
10/08/2009
Offline
Hello Tony!
My main goal was to show you, that the usage of JSLint can be really very helpful. So it would be good if you include JSLint in the process of the future release builds of jqGrid.
I also think (like you) that one should be very careful if one makes a lot of changes in a working code. But nevertheless please don’t forget to fix some clear bugs which I described in my two posts: like usage $s or ts instead of $t in grid.custom.js (see /blog/?page_id=393/bugs/more-bug-fixes-and-other-suggestions-found-with-respect-of-jslint-tool/) or missing "var" declarations. (Otherwise I had spend a lot of my time for nothing)
One more remark: I war at the beginning much more careful with changes of the "Equal" and "Not Equal" operators ("==" and "!==") to "Identically Equal" and "Not Identically Equal" ("===" and "!==="), which makes comparison without any type conversion. Then I make one test. I changed all "Equal" and "Not Equal" operators to "Identically Equal" and "Not Identically Equal" in grid.base.js. Then I tested my current program. Till now I didn’t found any problems! Now I am much more optimistic in the subject.
I see now a trend in JavaScript to usage more strict dialect of the language (see "strict more" in ECMAScript 5, for example, or http://ejohn.org/blog/ecmascri.....-and-more/). I find that jqGrid is now the best Grid plugin for jQuery. But to stay the best jqGrid in the future, you have to use the newest technology and look after (or sometimes follow) the last development trends.
Best regards
Oleg
19:59
Moderators
30/10/2007
Offline
Hello Oleg,
I have updated a lot of thing. We need to test it in order prevent unwantend behaviour.
Also Have found a lot of mistakes and etc.
Best Regards
Tony
For professional UI suites for Java Script and PHP visit us at our commercial products site - guriddo.net - by the very same guys that created jqGrid.
22:48
10/08/2009
Offline
Hello Tony!
I tested the current version from GitHub and found till now only one small problem.
I have one Grid where a column has formatter: 'showlink'. The data send from server have sometime null values. Previous version on jqGrid formated this value like an empty string. The current version from GitHub instead format 'null' value like clickable null. It seems to me that the line 235
shortly added inside of $.fn.fmatter.showlink function in the file jquery.fmatter.js should be removed. At least I don't undestand it's meaning.
Now you undestand, that working with JSLint tooks not so little fime. I hope you will not regret, that begin modifing of jqGrid corresponds of JSLint recomendations.
Inspide of large number of changes in jqGrid I found, that some bugs is not yet changed. For example, the line 160 of grid.custom.js looks the same as before:
but it should be fixed to
One more clear bug, which I described befor (see /blog/?page_id=393/bugs/more-bug-fixes-and-other-suggestions-found-with-respect-of-jslint-tool/) is in the lines 203-204 of grid.jqueryui.js. The code
selector.addClass(classname);
select.addClass(classname);
}
should be fixed to
selector.addClass(opts.classname);
select.addClass(opts.classname);
}
The line 19 of grid.subgrid.js:
should be fixed to
and the line 280 of grid.treegrid.js:
should be fixed to
The line 237 of jquery.fmatter.js:
should be also changed to
It's only the main not yet fixed problems described in my previous long post.
Moreover if we stay on grid.base.js and try to make this the most important file more clear, we could reduce JSLint recomendation if we change the order of functions in the grid.base.js file. For example, inside of addCell function (lines 392-397) in the line 394 will be used formatter function defined in the line 398. If one swap the order this two functions in the grid.base.js file one receive less warnings. It seems to me, that for all JavaScript engine and JSLink also it would be easyer to analyse the code after such changes. In one a little change position of the functions formatter, cellVal, updatepager, sortArrayData, beginReq, endReq in the code one receive much fewer warnings.
Function IntNum from the grid.base.js, used inside of grid.base.js only, has first capital character, which should be used for constructors only corresponds to the JavaScript name conversion. The same problem exists only with the function DaysArray from grid.common.js (defined in the line 591). I find the JSLint recomendation good. One can rename IntNum to intNum and DaysArray to daysArray and make all jqGrid function in the same name conversion.
Another small problem exists in lines 967-968:
format = format.split(/[/:_;.tTs-]/);
JSLint comment these line so: Unescaped '-'. It seems to me here we have almost the same problem like missing semicolon. All JavaScript engine which I knows will interpret these lines without any errors, but generally '-' character between '[' and ']' is used for difinition of ranges (like [A-Z]). So one '-' should be escaped to prevent it from being confused with a range hyphen. Of cause here we have no other characters after '-' before ']', so we have not a real bug. Nevetherless the lines
format = format.split(/[/:_;.tTs-]/);
I personally find more readable.
One more LSLint recommendation about lines 196 and 252:
and
I find also OK. The fixed lines
and
will looks more like the rest of the jqGrid code.
These lines probably come in jqGrid after the suggestion from the discussion /blog/?page_id=393/discussion/jqgrid-data-loading-performace/. One could improve these lines later if one bind onselectstart not to document, but to an element (see. http://plugins.jquery.com/file.....1.0.js.txt) or use different technic depend on browser (like
etc. see, for example, http://mediavrog.net/blog/2007.....t-per-css/ or functions disableSelection and enableSelection in lines 117-129 of jquery.ui.core.js).
One more small comment about the JSLint suggestion to the last line of grid.base.js code. How I interpret the recommendation is to replace code
$.jgrid = $.jgrid || {};
$.extend($.jgrid,{
…
});
})(jQuery);
to the code
$.jgrid = $.jgrid || {};
$.extend($.jgrid,{
…
});
}(jQuery));
You can deceide youself whether to follow this recomendation.
Sorry my post will be longer and longer. Better we can discuss other subjects later.
Best regards
Oleg
11:54
Moderators
30/10/2007
Offline
Hello Oleg,
Thanks. Most of them done.
Regards
Tony
For professional UI suites for Java Script and PHP visit us at our commercial products site - guriddo.net - by the very same guys that created jqGrid.
Most Users Ever Online: 715
Currently Online:
41 Guest(s)
Top Posters:
OlegK: 1255
markw65: 179
kobruleht: 144
phicarre: 132
YamilBracho: 124
Renso: 118
Member Stats:
Guest Posters: 447
Members: 11373
Moderators: 2
Admins: 1
Forum Stats:
Groups: 1
Forums: 8
Topics: 10592
Posts: 31289
Newest Members:
, razia, Prankie, psky, praveen neelam, greg.valainis@pa-tech.comModerators: tony: 7721, Rumen[Trirand]: 81
Administrators: admin: 66
