HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript portion of regexp tester

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptregexpportiontester

Problem

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 + '' + matched_string + '' + 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) {
$('#r

Solution

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 '

-
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 +
  '' + matched_string + '' +
  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();
+ (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:

%%CODEBLOCK_3%%

-
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:

%%CODEBLOCK_4%%

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.

%%CODEBLOCK_5%%

Code Snippets

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);
  // ...
}
if (!result) {
  result_spans.filter(".not_ok").show('fast');
  result_spans.filter(".ok").hide();
  return;
}
// reduced nesting
var matched_string = result.shift();
// ...
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();
}
(function(){
  // private scope for your code
}());
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;

Context

StackExchange Code Review Q#972, answer score: 4

Revisions (0)

No revisions yet.