patternjavascriptMinor
JavaScript portion of regexp tester
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
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
-
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:
-
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:
-
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%%
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.