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.

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();
  });
});
share|improve this question

2 Answers 2

up vote 3 down vote accepted

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();
share|improve this answer
    
Merci beaucoup Éric ! That's exactly the kind of feedback I was looking for :) I've updated the code on GitHub with your advices, except for the anonymous functions I've not yet removed (I'll do that when I add tests). I had tried to use JSLind but you're right, it will be more understandable after I read the Good Parts. –  Florent2 Feb 28 '11 at 15:58

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.

share|improve this answer
    
Interesting, thanks! –  Florent2 Mar 22 '11 at 12:46

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.