Help with javascript project

[Helbrax]

Hi. I currently run a "pet project" at http://www.jcritters.com.
Basically what I am looking for is anyone who is proficient in
javascript to go over the code(jcritter.js) and point out any areas of
improvement or any errors or best practices that need to be
addressed.

I am also looking for any feature suggestions that are within the
scope of the project.

 

[Thomas 'PointedEars' Lahn]

Hi. I currently run a "pet project" at http://www.jcritters.com.
Basically what I am looking for is anyone who is proficient in
javascript to go over the code(jcritter.js) and point out any areas of
improvement or any errors or best practices that need to be
addressed.

Use of standardized features without feature test:

// jcritters.js:47 and other lines
var bubbleDiv = document.createElement('div');

Constructor-style identifier without constructor use:

// jcritter.js:83 et al
critterObject.DrawAgent = function()

Use of proprietary features without feature test:

// jcritter.js:84 et al.
critterImg.style.width = agent.width + 'px';

// jcritter.js:167
window.onscroll = null;

// jcritter.js:286 and following lines
ajaxObject.setRequestHeader("Content-type",
"application/x-www-form-urlencoded");

Use of error-prone window.attachEvent/window.detachEvent, also lacking
sufficient feature test:

// jcritter.js:157 and other lines
else if(window.attachEvent)

http://www.quirksmode.org/blog/archives/2005/08/addevent_consid.html

Inefficient loops:

// jcritter.js:333 and probably other lines
for(var i = 0; i < reqObj.length; i++)

False assumptions:

// jcritters.js:353 and other lines
return window.innerWidth || (document.documentElement.clientWidth ||
document.body.clientWidth)

(Either property is proprietary, yet you assume that one must evaluate to a
non-zero number.)

And probably I have forgotten something.


HTH

PointedEars

 

[Helbrax]

Use of standardized features without feature test:

// jcritters.js:47 and other lines
var bubbleDiv = document.createElement('div');

Ok. I've already begun to address this in the next version. I'm
going to run as many feature tests as possible in the initial
constructor

Constructor-style identifier without constructor use:

// jcritter.js:83 et al
critterObject.DrawAgent = function()

Hmm..how should I assign the methods? Like this?
critterObjcect.DrawAgent = doThis;
function doThis(args)
{
....
}

Use of proprietary features without feature test:

// jcritter.js:84 et al.
critterImg.style.width = agent.width + 'px';

// jcritter.js:167
window.onscroll = null;

// jcritter.js:286 and following lines
ajaxObject.setRequestHeader("Content-type",
"application/x-www-form-urlencoded");

The ajax and cookie codes are going bye bye anyway. The other feature
tests will be added.

Use of error-prone window.attachEvent/window.detachEvent, also lacking
sufficient feature test:

// jcritter.js:157 and other lines
else if(window.attachEvent)

http://www.quirksmode.org/blog/archives/2005/08/addevent_consid.html

I'll do more research on the attachEvent model.

Inefficient loops:

// jcritter.js:333 and probably other lines
for(var i = 0; i < reqObj.length; i++)

This loop will be going away, but why is it inefficient? Should I do:
for(...) if (...)
{

}

False assumptions:

// jcritters.js:353 and other lines
return window.innerWidth || (document.documentElement.clientWidth ||
document.body.clientWidth)

(Either property is proprietary, yet you assume that one must evaluate to a
non-zero number.)

I'll address this with the features in the constructor. If neither of
these is there, there is no sense in continuing, as the critter will
not display correctly.

And probably I have forgotten something.

I wouldn't doubt it. I'm not a very good javascript programmer.

HTH

PointedEars

My main goal is to support most modern browsers, which I've done(I've
tested on Firefox, SeaMonkey, Flock, IE6, IE7, Opera, Konquerer,
Safari and several others), but I'm happy to see there is room for
improvment.

 

[Helbrax]

Hmm...It looks like the biggest issue with attachEvent is that it
doesn't implicitly remove it's event when the window unloads. This
only seems to be a problem with IE6(and possibly lower). IE7 seems to
remove the memory(at least in my tests). Calling object.Destroy() in
an onunload event in IE6 seems to clear out the memory just fine.

 

[Thomas 'PointedEars' Lahn]

Hmm..how should I assign the methods? Like this?
critterObjcect.DrawAgent = doThis;
function doThis(args)
{
...
}

No, I was talking about the *identifier* instead:

critterObject.drawAgent = function()
{
// ...
};

Besides, on second thought, I don't understand your using this
hard-to-maintain `critterObject' reference instead of `this' *everywhere*.
And ISTM you are defining too many methods in the constructor where a
prototype method would have been better. Example:

function Foo(nBar)
{
if ((this.successful = !isNaN(nBar)))
{
this.bar = nBar;
}
}

Foo.prototype = {
constructor: Foo,
bar: 23,

baz: function()
{
window.alert(this.bar);
}
};

var foo = new Foo(42);
if (foo.successful)
{
foo.baz();
}

This loop will be going away, but why is it inefficient?

Because reqObj.length is evaluated in every loop.

Should I do:
for(...) if (...)
{

}

Certainly not, but:

for (var i = 0, len = somelength; i < len; i++)

In cases where order is not important (i.e. vice-versa iteration is OK),

for (var i = somelength; i--

suffices. We have discussed this here before.

I'll address this with the features in the constructor. If neither of
these is there, there is no sense in continuing, as the critter will
not display correctly.

Just in case you are unaware of that:

As you usually should not return a value from the constructor, there are
two possibilities for that: You throw an exception which makes your code
incompatible to JavaScript < 1.5, JScript < 5.0, ECMAScript < 3, see
http://PointedEars.de/scripts/es-matrix/

Or you define/test a property of the newly created object to
specify/determine that something when wrong during construction. See above.


Please include an empty line between quotation and new text next time.


PointedEars

 

[Helbrax]

No, I was talking about the *identifier* instead:

critterObject.drawAgent = function()
{
// ...
};

I didn't realize that was a requirement or a best practice.

Besides, on second thought, I don't understand your using this
hard-to-maintain `critterObject' reference instead of `this' *everywhere*.
And ISTM you are defining too many methods in the constructor where a
prototype method would have been better. Example:

function Foo(nBar)
{
if ((this.successful = !isNaN(nBar)))
{
this.bar = nBar;
}
}

Foo.prototype = {
constructor: Foo,
bar: 23,

baz: function()
{
window.alert(this.bar);
}
};

var foo = new Foo(42);
if (foo.successful)
{
foo.baz();
}

I'm not sure why critterObject is harder to maintain than 'this'. I
always know what critterObject is referring too, while 'this' can
change depending on if it's in another function, closure, etc. I
don't currently have any instances where I could have more than one
"this'", but if I didn't I wouldn't have to worry about what 'this' is
really referring to.

Yeah, I see what you mean about taking my function out of the
constructor code and protyping instead.

Because reqObj.length is evaluated in every loop.



Certainly not, but:

for (var i = 0, len = somelength; i < len; i++)

In cases where order is not important (i.e. vice-versa iteration is OK),

for (var i = somelength; i--

suffices. We have discussed this here before.

Ok. Code is going away, but good to know.

Just in case you are unaware of that:

As you usually should not return a value from the constructor, there are
two possibilities for that: You throw an exception which makes your code
incompatible to JavaScript < 1.5, JScript < 5.0, ECMAScript < 3, seehttp://PointedEars.de/scripts/es-matrix/

Or you define/test a property of the newly created object to
specify/determine that something when wrong during construction. See above.

I wasn't planning on returning a value from the constructor. The
current version I'm working on already has a few feature tests in it,
and they set a property of 'supported' that evaluates to true or
false.

Please include an empty line between quotation and new text next time.

I did, at least partially. I was trying to type and play with my 2
year old at the same time, so I'll chalk that one up to distraction.

 

[Thomas 'PointedEars' Lahn]

[...] Thomas 'PointedEars' Lahn [...] wrote:
Constructor-style identifier without constructor use:
// jcritter.js:83 et al
critterObject.DrawAgent = function()
Hmm..how should I assign the methods? Like this?
critterObjcect.DrawAgent = doThis;
function doThis(args)
{
...
}

No, I was talking about the *identifier* instead:

critterObject.drawAgent = function()
{
// ...
};

I didn't realize that was a requirement or a best practice.

It is recommended practice, not a requirement.

Besides, on second thought, I don't understand your using this
hard-to-maintain `critterObject' reference instead of `this' *everywhere*.
And ISTM you are defining too many methods in the constructor where a
prototype method would have been better. Example:
[...]

I'm not sure why critterObject is harder to maintain than 'this'.

Suppose you change the identifier of the variable ...

I always know what critterObject is referring too, while 'this' can
change depending on if it's in another function, closure, etc.

I didn't debate that sometimes a local variable is necessary. But a) would
I then use something short, like `me' instead, and b) is it not necessary
everywhere. In fact, by using the variable everywhere instead of `this',
you are creating closures where there do not need to be any. Closures
easily lead to circular references, which when host objects are involved
cause a memory leak in MSHTML-based UAs if not handled accordingly.

I don't currently have any instances where I could have more than one
"this'", but if I didn't I wouldn't have to worry about what 'this' is
really referring to.

Ignorance is bliss ... until it bites back ;-)

I did, at least partially. I was trying to type and play with my 2
year old at the same time, so I'll chalk that one up to distraction.

ACK. (JFYI: Trimming quotes is much more important than the empty line.)


PointedEars

 

[Helbrax]

And ISTM you are defining too many methods in the constructor where a

prototype method would have been better.

I'm using a lot of private variables that I do not want to be exposed
to the object creator. If I used a prototype model, how would I keep
these private, but still pass them around to my methods that are added
via prototype:

//I want foobarious to be private to the Foo object, but still be
accesible to the methods added via prototype
function Foo(bar)
{
var foobarious = 'ham';
}

Foo.prototype.sandWich = function()
{
alert(foobarious); //This won't work, and I didn't shouldn't.
}

//I want foobarious to be private to the Foo object, but still be
accesible to the methods added via prototype
function Foo(bar)
{
this.foobarious = 'ham';
}

Foo.prototype.sandWich = function()
{
alert(this.foobarious); //This works, as it should
}

In the working example though, foobarious is exposed, which I don't
want it to be:
var blah = new Foo();
alert(blah.foobarious); //oops I don't want you to be able to see
this.