SitePoint Sponsor

User Tag List

Results 1 to 6 of 6
  1. #1
    SitePoint Member
    Join Date
    Dec 2007
    Posts
    4
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    [CODE REVIEW] help with quality of code

    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?

    Code JavaScript:
    $(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?
    Last edited by Mittineague; Mar 11, 2011 at 23:10.

  2. #2
    SitePoint Wizard bronze trophy chris.upjohn's Avatar
    Join Date
    Apr 2010
    Location
    Melbourne, AU
    Posts
    2,183
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Try the below

    Code JavaScript:
    $(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 $.ready() so all you need to use is

    Code JavaScript:
    $(function() {
        // jQuery code here
    });
    Blog/Portfolio | Evolution Xtreme | DFG Design | DFG Hosting | CSS-Tricks | Stack Overflow | Paul Irish
    Having lame problems with your code? Let us help by using a jsFiddle

  3. #3
    SitePoint Member
    Join Date
    Dec 2007
    Posts
    4
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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. #4
    SitePoint Wizard bronze trophy chris.upjohn's Avatar
    Join Date
    Apr 2010
    Location
    Melbourne, AU
    Posts
    2,183
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    _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 $(this) needs to be called it depends on whether the object needs to be stored within a variable.
    Blog/Portfolio | Evolution Xtreme | DFG Design | DFG Hosting | CSS-Tricks | Stack Overflow | Paul Irish
    Having lame problems with your code? Let us help by using a jsFiddle

  5. #5
    SitePoint Member
    Join Date
    Dec 2007
    Posts
    4
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks!

  6. #6
    SitePoint Member
    Join Date
    Dec 2007
    Posts
    4
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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();


Tags for this Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •