patternjavascriptMinor
Do I always have to return something in an anonymous function?
Viewed 0 times
somethingreturnanonymousalwaysfunctionhave
Problem
I have inherited this snippet of jQuery JavaScript and am currently brushing up on my jQuery. NetBeans IDE complains that
I am aware that the
My question is - do I always have to return something in an anonymous function? I.e. is NetBeans right to complain?
Anonymous function does not always return a value. So there is not always an explicit exit point.I am aware that the
$() shortcut is not being used. This is because it is running within a WordPress environment and needs to be in no-conflict mode.function init() {
jQuery("#post").submit(function(_e) {
// Make sure question is supplied
var contents;
if(window.tinyMCE && document.getElementById("content").style.display=="none") { // If visual mode is activated.
contents = tinyMCE.get("content").getContent();
} else {
contents = document.getElementById("content").value;
}
if(!contents) {
alert(msg_enter_question);
_e.preventDefault();
_e.stopPropagation();
return true;
}
// We must have atleast 2 answers.
var answer_count = 0
jQuery(".answer").each(function() {
if(this.value) answer_count++;
});
//if(answer_count ");
// _e.preventDefault();
// _e.stopPropagation();
// return true;
//}
//A correct answer must be selected.
var correct_answer_selected = false;
jQuery(".correct_answer").each( function(_e) {
if(this.checked) {
correct_answer_selected = true;
return true;
}
});
if(!correct_answer_selected) {
alert(msg_correct_answer);
_e.preventDefault();
_e.stopPropagation();
}
});
}
jQuery(document).ready(init);My question is - do I always have to return something in an anonymous function? I.e. is NetBeans right to complain?
Solution
While I understand how it may seem a pointless little nitpick for such simple code to have to have a
Further recommendations
-
Wrap the code in an IIFE (and add
On top of other things this allows you to use a
-
There is no reason to name the init function that you are just passing to
-
Cache the jQuery elements.
-
Pass in global variables used to the IIFE (
-
-
-
This:
is better written as:
But it is even better to be commented out or otherwise removed since you aren't using it anywhere.
-
Similarly:
should be:
-
Move variables to the top of their respective functions (personal preference in agreement with JSLint).
-
Returning
may as well be:
Most obvious there is that the
As this same if statement is used twice (with different values) I would prefer to pull it out into a function:
This leaves:
-
The problem of not all paths returning something is still here, so change the last line to
-
If you inline
The Full Monty then becomes (after some very minor spacing changes to pass jslint):
return something from all code paths if it has one from any, pretty much every thing that a reviewer could say something about can be seen the same way. Individually the rules like the one Netbeans wants to enforce here don't do much, but when all followed together they make for much more maintainable code (especially after letting it sit there until you have forgotten about it for 16 months).Further recommendations
-
Wrap the code in an IIFE (and add
'use strict';)On top of other things this allows you to use a
$ for jQuery here and not worry about messing with any conflict issues.-
There is no reason to name the init function that you are just passing to
$.ready; inline it.-
Cache the jQuery elements.
-
Pass in global variables used to the IIFE (
window, alert, msg_enter_question, msg_correct_answer).-
document.getElementById("content").style.display has too many . I'd rewrite it as $content.css('display') after caching the element. Better still:!$content.is(':visible')-
_e is an awful variable name; that underscore adds nothing to it but makes it more annoying to type. Rename to e.-
This:
var answer_count = 0
$(".answer").each(function() {
if(this.value) answer_count++;
});is better written as:
var answer_count = $answers.filter(function () { return this.value !== ''; }).length;But it is even better to be commented out or otherwise removed since you aren't using it anywhere.
-
Similarly:
var correct_answer_selected = false;
$(".correct_answer").each( function(_e) {
if(this.checked) {
correct_answer_selected = true;
return true;
}
});should be:
var correct_answer_selected = $correctAnswers.is(':checked');-
Move variables to the top of their respective functions (personal preference in agreement with JSLint).
-
Returning
false from a handler is equivalent to calling both .preventDefault() and .stopPropagation() on the event object. Thus:if(!contents) {
alert(msg_enter_question);
e.preventDefault();
e.stopPropagation();
return true;
}may as well be:
if(!contents) {
alert(msg_enter_question);
return false;
}Most obvious there is that the
return true statement is exactly not what is actually happening and thus is wrong. Since I would prefer to be explicit I would probably do this:if(!contents) {
alert(msg_enter_question);
e.preventDefault();
e.stopPropagation();
return false;
}As this same if statement is used twice (with different values) I would prefer to pull it out into a function:
function check(item, e, msg) {
if (!item) {
alert(msg);
e.preventDefault();
e.stopPropagation();
return false;
}
return true;
}This leaves:
//...
if(!check(contents, e, msg_enter_question)) {
return false;
}
correct_answer_selected = $correctAnswers.is(':checked');
check(correct_answer_selected, e, msg_correct_answer);
}-
The problem of not all paths returning something is still here, so change the last line to
return check....-
If you inline
correct_answer_selected then the pattern if (!x) { return false; } return y; logic can be replaced with return x && y:return check(contents, e, msg_enter_question) &&
check($correctAnswers.is(':checked'), e, msg_correct_answer);The Full Monty then becomes (after some very minor spacing changes to pass jslint):
(function ($, window, alert, msg_enter_question, msg_correct_answer) {
'use strict';
$(function () {
var $correctAnswers = $(".correct_answer"),
$content = $('#content');
function check(item, e, msg) {
if (!item) {
alert(msg);
e.preventDefault();
e.stopPropagation();
return false;
}
return true;
}
$("#post").submit(function (e) {
// Make sure question is supplied
var contents;
if (window.tinyMCE && !$content.is(':visible')) { // If visual mode is activated.
contents = window.tinyMCE.get("content").getContent();
} else {
contents = $content.val();
}
return check(contents, e, msg_enter_question) &&
check($correctAnswers.is(':checked'), e, msg_correct_answer); //A correct answer must be selected.
});
});
}(jQuery, window, alert, msg_enter_question, msg_correct_answer));Code Snippets
!$content.is(':visible')var answer_count = 0
$(".answer").each(function() {
if(this.value) answer_count++;
});var answer_count = $answers.filter(function () { return this.value !== ''; }).length;var correct_answer_selected = false;
$(".correct_answer").each( function(_e) {
if(this.checked) {
correct_answer_selected = true;
return true;
}
});var correct_answer_selected = $correctAnswers.is(':checked');Context
StackExchange Code Review Q#656, answer score: 6
Revisions (0)
No revisions yet.