current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I feel like there is too much repetitive code going on here. All I am doing is doing a basic regex match for a string in the URL. If a match is found, I find an li with a class (.index, .grid, .type) and add the active class. This is just for my main nav in an attempt to make it somewhat dynamic. However, I feel like there is a more efficient way to code this.

$( document ).ready(function() {
var myLocation = window.location.href;
var index = /index/i;
var grid = /grid/i;
var type = /type/i;
var urlIndex = convertURL.match(index);
var urlGrid = convertURL.match(grid);
var urlType = convertURL.match(type);
if(urlIndex) {
$('.index').addClass('active'); 
}else if(urlGrid) {
$('.grid').addClass('active');
}else if(urlType) {
$('.type').addClass('active');
}

});
share|improve this question

3 Answers 3

up vote 4 down vote

Much like elclanrs' solution, just slightly more "plain"

$(function () {
  var classes = ['index', 'grid', 'type'],
      url = window.location.href.toLowerCase();
  for(var i = 0, l = classes.length ; i < l ; i++ ) {
    if(url.indexOf(classes[i]) !== -1) {
      $('.' + classes[i]).addClass('active');
      break; // only first matching class is considered
    }
  }
});
share|improve this answer
    
so without the break, all the li's would get the class "active" right? –  user3259232 May 26 at 1:11
    
@user3259232 All the ones with a class matching the URL would, yes. E.g. foo.com/bar/index/how-to-griddle.html would activate the index and grid items, but not the type ones.. –  David Harkness May 26 at 1:22
    
@user3259232 Without the break, the code would work like a series of ifs, instead of an if.. else if structure like your code. I.e. without the break a url like example.com/index/grid/type/xyz would cause it to act the same as $(".index, .grid, .type").addClass("active") because all three classes would get matched. But with the break, only the first matching class will be considered, so it's like $(".index").addClass("active") –  Flambino May 26 at 14:23
    
@Flambino My comment was an answer to user3259232's comment about removing the break. –  David Harkness May 26 at 18:12
    
@DavidHarkness Makes sense. Sorry - failed to read it in its proper context –  Flambino May 26 at 19:55
up vote 3 down vote

I think you meant myLocation in place of convertURL.

You don't need that many variables, a typical refactoring might look like this:

// Short version of $(document).ready(fn)
$(function() {
  var loc = window.location.href;
  if (/index/i.test(loc)) {
    $('.index').addClass('active'); 
  } else if (/grid/i.test(loc)) {
    $('.grid').addClass('active');
  } else if (/type/i.test(loc)) {
    $('.type').addClass('active');
  }
});

But I would go for a less imperative solution, without the use of regex, for example:

$(function() {
  var loc = window.location.href;
  var classes = ['index', 'grid', 'type'];
  var isMatch = function(x){return loc.toLowerCase().indexOf(x)>-1};
  $('.'+ classes.filter(isMatch)[0]).addClass('active');
});

Note that you can go even further an cache the lowercased url, but you may want to use the original variable intact later in the code.

share|improve this answer
1  
The first rewrite is missing the else part of the original else if tests. This could activate up to three items. In the second, you may as well convert the URL to lowercase when pulling it from window.location.href instead of repeating the process for every indexOf call. –  David Harkness May 26 at 1:01
    
@DavidHarkness: yes, I edited and made them if without realizing. And yes, I thought about doing toLowerCase above but OP might want to use the url in its original shape later on. –  elclanrs May 26 at 1:01
    
Keep in mind also that Array.prototype.filter is defined in ES5. You can use es5-shim for older browsers. –  David Harkness May 26 at 1:04
up vote 1 down vote 
$(document).ready(function () {

    // use single var per function, 
    // good for minimizing and other stuff
    var

    i,

    // new string literal, not String object
    convertURL = '' + window.location,

    // the array of strings keeps only the difference 
    // from the repetitive code
    classes = ['index', 'grid', 'type'],

    // using functions with proper arguments reduces repetitivness
    matches = function (regex) {
        return convertURL.match(new RegExp(regex, 'i'));
    }

    // var
    ;

    // always use += instead of ++ -> makes for clear intention
    for (i = 0; i < classes.length; i += 1) {
        if (matches(classes[i])) {
            // returning out of this function stops iteration
            return $('.' + classes[i]).addClass('active');
        }
    }

});
share|improve this answer
    
It seems a bit wasteful to use regex when a simple indexOf on the lowercased URL would suffice. –  David Harkness May 26 at 1:05
    
At this moment I don't see it as a concern because DOM manipulation is so much slower that it will negate all of the micro-optimizations :/ –  Azder May 26 at 1:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.