JavaScript portion of regexp tester

Question

I'm quite new to JavaScript, and I'd like a review of the code structure and syntax. It serves this little online regexp test (still a work in progress).

The whole code (JavaScript, CSS & HTML) is on GitHub.

application.js:

function update_result_for(input, regexp_value) {
  var input_value   = input.val();
  var result_spans  = input.parent().children('span');        
  if(!input_value || !$('#regexp').val()) {
    result_spans.hide();
  } else {
    var regexp = new RegExp(regexp_value);
    var result = regexp.exec(input_value);
    if(result) {
      var matched_string = result.shift();

      var submatches_list_string = jQuery.map(result, function(submatch, index) {
        return '$' + (index + 1) + ' = ' + submatch;
      }).join('; ');

      var regexp_to_highlight_matched_string          = new RegExp('(.*)' + matched_string + '(.*)');
      var regexp_to_highlight_matched_string_result   = regexp_to_highlight_matched_string.exec(input_value);
      var before_matched_string                       = regexp_to_highlight_matched_string_result[1];
      var after_matched_string                        = regexp_to_highlight_matched_string_result[2];
      var input_value_with_matched_string_highlighted   = 'matched: ' + before_matched_string + '<span class="matched">' + matched_string + '</span>' + after_matched_string;

      result_spans.filter(".submatches").text(submatches_list_string);
      result_spans.filter(".match").html(input_value_with_matched_string_highlighted);
      result_spans.filter(".ok").show('fast');            
      result_spans.filter(".not_ok").hide();
    } else {
      result_spans.filter(".not_ok").show('fast');
      result_spans.filter(".ok").hide();
    }
  }
}       

// from http://www.scottklarr.com/topic/126/how-to-create-ctrl-key-shortcuts-in-javascript/
var isCtrl = false;
$(document).keyup(function (e) {
    if(e.which === 17) isCtrl=false;
}).keydown(function (e) {
    if(e.which === 17) isCtrl=true;
    if(e.which === 69 && isCtrl) {
    $('#regexp').focus();          
        return false;
    }
});

$(document).ready(function() {
  $('#regexp').focus();
  $('span.result').hide();
  $('input:not(#regexp)').live("keyup", function() {
    update_result_for($(this), $('#regexp').val());
  });        
  $('input#regexp').keyup(function() {
    $('input:not(#regexp)').each(function(i) {
      update_result_for($(this), $('#regexp').val());
    });          
  });
  $('a.add_example').click(function() {
    new_example = $('div#examples p:last').clone();
    new_example.children('input').attr('value', '');
    new_example.children('span').hide();
    new_example.insertBefore($(this));
    new_example.children("input").focus();
  });
});

Answer 1

This code looks fine to me for a toy project. I especially appreciate the long names for identifiers.

Now, it could look better:

• with more comments. You should describe the intent (the why behind the how) and the expected use case for the function: type and range of values in parameters, one line to describe what it does, and one more paragraph for details if needed.

• my personal preference would be to use camelCase instead of underscore_between_words for long names. Although this is open to discussion.

• a little trick: end the if branch with return; to avoid nesting all remaining code in the else branch:

  function update_result_for(input, regexp_value) {
    var input_value   = input.val();
    var result_spans  = input.parent().children('span');        
    if(!input_value || !$('#regexp').val()) {
      result_spans.hide();
      return; // return as soon as possible to avoid deep nesting
    }
    // no need for else
    var regexp = new RegExp(regexp_value);
    var result = regexp.exec(input_value);
    // ...
  }
  

• in the same vein, treat exceptional cases first and normal cases after. This is a useful convention which helps the reader, and the exception handling is usually shorter (or should be extracted to a separate function if longer) which avoids long runs of nested code:

  if (!result) {
    result_spans.filter(".not_ok").show('fast');
    result_spans.filter(".ok").hide();
    return;
  }
  // reduced nesting
  var matched_string = result.shift();
  // ...
  

• only use anonymous functions when you actually need a closure with access to the context: the intent of the function will be clearer with a name, you will avoid nesting, and may reuse the function more easily including for unit testing. There are many anonymous functions in your code, which makes it harder to understand:

  function(submatch, index) {
    return '$' + (index + 1) + ' = ' + submatch;
  })
  
  function(e) {
    if(e.which === 17) isCtrl=false;
  }
  
  function(e) {
    if(e.which === 17) isCtrl=true;
      if(e.which === 69 && isCtrl) {
      $('#regexp').focus();          
          return false;
      }
  })
  
  function() {
    $('#regexp').focus();
    $('span.result').hide();
    // ...
  }
  
  function() {
    update_result_for($(this), $('#regexp').val());
  }
  
  function() {
    $('input:not(#regexp)').each(function(i) {
      update_result_for($(this), $('#regexp').val());
    });          
  }
  
  function() {
    new_example = $('div#examples p:last').clone();
    new_example.children('input').attr('value', '');
    new_example.children('span').hide();
    new_example.insertBefore($(this));
    new_example.children("input").focus();
  }
  

• once you define more than one function, you should wrap your code in a closure to avoid cluttering the global namespace, following the Module Pattern:

  (function(){
    // private scope for your code
  }());
  

• break long lines to fit in about 80 characters to avoid the need for scrolling horizontally in typical console windows and in code areas on this site:

  var regexp_to_highlight_matched_string =
    new RegExp('(.*)' + matched_string + '(.*)');
  var regexp_to_highlight_matched_string_result =
    regexp_to_highlight_matched_string.exec(input_value);
  var before_matched_string =
    regexp_to_highlight_matched_string_result[1];
  var after_matched_string =
    regexp_to_highlight_matched_string_result[2];
  var input_value_with_matched_string_highlighted =
    'matched: ' +
    before_matched_string +
    '<span class="matched">' + matched_string + '</span>' +
    after_matched_string;
  

To go further, my advice would be to read "JavaScript: The Good Parts" and start using JSLint, in this order: this is an enlightening experience on your way to mastering JavaScript. The other way round, using the tool without understanding the mindset of its author, is very frustrating.

I ran JSLint on your code. It has one critical complaint hidden among hair splittings: the declaration of new_example is missing, it is therefore a global variable which is susceptible to result in unexpected bugs.

   // var keyword added:
   var new_example = $('div#examples p:last').clone();

Answer 2

Another tool you can use today:

Google's own Closure Compiler from Google, which is a tool for making JavaScript download and run faster. It parses your JavaScript, analyzes it, removes dead code and rewrites and minimizes what's left. It also checks syntax, variable references, and types, and warns about common JavaScript pitfalls.

There's a online version of this tool too, a Firebug plugin, and is already part of PageSpeed and there's also a JavaScript Linter as part of the Closure Tools.