[CODE REVIEW] help with quality of code

JavaScript
1

I’m new to programming and I’ve been able to hammer out some code that works, but I have no idea if there are better ways to have coded this. Could you please take a look at my code and tell me how to make it better?

$(document).ready(function(){

	// get text from all agenda and minutes links and add title tag for accessibility
       // hrefs can be in various folders, but the first part of the file name should be yyyy-mm-dd
      // e.g. <a href="files/agendas/2010-12-10-IITCag.docx">agenda</a>
      // or   <a href="files/mins/2010-12-10-IITCmins.docx">minutes</a>
	
	var meetingAnchor = $("#meetings a");
	var thisHref,
		thisDate,
		thisDateStart,
		titleText;

	meetingAnchor.each(function(i) {
		var myThis = $(this);
		var anchorText = myThis.text().toLowerCase().trim();
		if ( anchorText == 'agenda' || anchorText == 'minutes' ) {
			thisHref = myThis.attr("href");
			thisDateStart = thisHref.search(/20/);
			thisDate = thisHref.slice(thisDateStart);
			titleText = anchorText + " for " + thisDate.substring(5, 7) + "/" + thisDate.substring(8, 10) + "/" + thisDate.substring(0, 4);
			myThis.attr('title', titleText);
		}
  	}); // end of processing each anchor
  	
}); // end of document.ready function

One thing I’m really not sure of is where to declare my variables? Do they need to be inside the .each() function or outside of it?

2

Try the below

$(function() {
    // get text from all agenda and minutes links and add title tag for accessibility
    // hrefs can be in various folders, but the first part of the file name should be yyyy-mm-dd
    // e.g. <a href="files/agendas/2010-12-10-IITCag.docx">agenda</a>
    // or <a href="files/mins/2010-12-10-IITCmins.docx">minutes</a>

    var meetings = $('a', '#meetings');
    
    meetings.each(function() {
        var _this = $(this), text = _this.text().toLowerCase().trim();
        
        if (text == 'agenda' || text == 'minutes') {
            var href = _this.attr('href'), date = href.match(/[0-9+]{4}-[0-9+]{2}-[0-9+]{2}/), dateSplit = date[0].split('-');
            var titleText = text + ' for ' + dateSplit[2] + '/' + dateSplit[1] + '/' + dateSplit[0];
            _this.attr('title', titleText);
        }
    }); // end of processing each anchor
});

In my personal opinion declaring the vars inside the loop is better as your not storing the values for further use outside the loop. Also jQuery added the ability to use a shorthand declaration of $.[B]ready/B so all you need to use is

$(function() {
    // jQuery code here
});
3

Thank you! This is a lot cleaner and makes more sense. Awesome!

A followup question…

You used _this as the variable name for storing $(this).

Is that the variable name most people use for $(this)?

It seems like so many people would store this, I was just wondering what others preferred for this.

4

_this is just how i like doing it, i have seen a few other people do this including a few users from this site. Depending on the size of the loop and how many times $COLOR=#66CC66[/COLOR] needs to be called it depends on whether the object needs to be stored within a variable.

5

Thanks!

6

I was getting an “Object doesn’t support this property or method” error in IE 7 and 8. I narrowed this down to the trim() method and discovered the IE 7 and 8 don’t support trim(). So I changed the text variable declaration to:

text = jQuery.trim(_this.text()).toLowerCase();