ansaurus

Question

Answer 1

+2 A:

  //a list of comments
  this.comments = [];

I think your comments are occasionally not needed. IE: it's pretty obvious that that is a list, and probably holds comments.

This is one of those times where the code (variable names, specifically) is clear enough where a comment is unnecessary.

nilamo 2009-07-04 21:03:34

Honestly, I don't think 90% of his comments are needed.

musicfreak 2009-07-04 21:06:16

That's a fair criticism. A guy who taught me to program really drove it into me to write at least one comment per function, and I'm fairly religious about that, but my comments aren't always that useful. I've gotten that critique from colleagues before too.

Andrew Johnson 2009-07-04 21:10:29

Comments are only useful if you can read the code below without going cross-eyed

Aiden Bell 2009-07-04 21:13:56

Or accurate? Where the comment says, "returns the HTML for the button that reveals the comment form", I'd be expecting the method to return a string. It actually returns a DOM element. (Or am I the only one who thinks HTML ==> String?)

Dan Breslau 2009-07-04 21:14:51

Answer 2

+2 A:

After looking over some of your code, I have a few comments:

Div ID:

I notice that in the constructor for your widget, you use the line:

if (id) this.div = document.getElementById(id);

to have a DIV ID passed in to be used in your class. However, you then go on to check whether this property exists several times after this in your code with the line:

if (typeof this.div == 'undefined') return;

Perhaps you want to check whether a valid DIV is passed in in the first place, which will save you from checking later on. Also, you might want to implement this so that a DIV ID can be passed in, or the actual DIV element itself.

DOM Helpers:

I also notice that you have several occurrences of createElement() and appendChild(). DOM manipulation through Javascript isn't among the most concise of tasks. If you don't want to use libraries to help you with this, that's fine, but you may want to consider using some kind of helper functions, such as:

function create(tag, parent)
{
    var el = document.createElement(tag);   
    append(el, parent);

    return el;
}

function append(el, parent)
{
    if(typeof(el)!="undefined"&&typeof(parent)!="undefined")
        parent.appendChild(el);
}

You could also add these to your class's prototype. This would decrease the amount of code that you're writing, so would help to keep the size of your widget to as minimal as possible. It would also make it easier to read.

Property helpers:

I also noticed that you have lots of property assignments going on, such as:

textarea.className = 'replyText'

and

input.type = 'button';

Since that you are assigning one property per line, and keep having to write:

element.property = value;

over and over again, this increases the size of your code, and also, again, makes it harder to read. I would suggest that you add some kind of method to add properties from an object literal - I often do this when setting multiple styles on elements. Something like:

function setProperties(el, properties)
{
    for(var p in properties)
    {
        el[p] = properties[p];
    }
}

Code such as this:

var textarea = document.createElement('textarea');
textarea.className = 'replyText'
textarea.onclick = expandCommentBox;
textarea.name = 'replyDiv' + replyA.replyID;
textarea.value = "Your reply goes here.";

Would then be rewritten as something like:

var textarea = document.createElement('textarea');
setProperties(textarea, {className: 'replyText',
                         onClick: expandCommentBox,
                         name: 'replyDiv' + replyA.replyID,
                         value: "Your reply goes here."});

As you can see, it drastically reduces the number of lines used (I placed each property in the object literal on separate lines to make it more readable) and also reduces the amount of code you are writing, which makes it more lightweight. You could also add it as an extra optional parameter to a DOM create element method as I discussed above, if you implemented one.

Perspx 2009-07-04 21:21:38

I think I have to disagree with the property helpers there. It's not *that* much less code and I don't really think it makes it more readable. It only saves you from writing the word textarea, something that can already be done using a with statement.

Andy E 2009-07-05 08:55:10

I guess it depends on the context - in some places I think that it would be worth it, when he sets several properties which can take up five lines, and are repetitive. If the object literal was an optional third argument for a scenario where he creates the element, appends it to another element, AND sets properties on it, for example, it would be very useful, and would combine 7 lines of code into 1 or 2.

Perspx 2009-07-05 14:31:38

ansaurus

tags:

views:

answers:

Please Review my JavaScript Code

related questions