6
\$\begingroup\$

I have inherited this snippet of jQuery JavaScript and am currently brushing up on my jQuery. NetBeans IDE complains that 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 < 2) {
        //  alert("<?php //_e("Please enter atleast two answers"); ?>");
        //  _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?

\$\endgroup\$
2
  • \$\begingroup\$ it depends, does the function call require the function to return a value. such as: value = (function(){return 1})(document); \$\endgroup\$
    – RobertPitt
    Feb 6, 2011 at 16:22
  • \$\begingroup\$ No, it doesn't require an actual value to be returned. It just checks if a form field is populated or whether at least one of several checkboxes is checked. \$\endgroup\$
    – codecowboy
    Feb 7, 2011 at 6:26

2 Answers 2

6
\$\begingroup\$

While I understand how it may seem a pointless little nitpick for such simple code to have to have a 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

  1. 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.

  2. There is no reason to name the init function that you are just passing to $.ready; inline it.

  3. Cache the jQuery elements.

  4. Pass in global variables used to the IIFE (window, alert, msg_enter_question, msg_correct_answer).

  5. 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')
    
  6. _e is an awful variable name; that underscore adds nothing to it but makes it more annoying to type. Rename to e.

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

  8. 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');
    
  9. Move variables to the top of their respective functions (personal preference in agreement with JSLint).

  10. 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);
    }
    
  11. The problem of not all paths returning something is still here, so change the last line to return check....

  12. 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));
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Upvote 'cause it's a fantastic answer though I disagree with #7 'cause your suggestion is a bit harder to read. \$\endgroup\$
    – ari gold
    Mar 6, 2014 at 22:25
2
\$\begingroup\$

Netbeans is right to complain about this (there are some details on the website about it but they are a bit sparse). Also this isn't Netbeans specific, for example Firefox with strict-mode enabled will warn about this also.

So in:

jQuery(".correct_answer").each( function(_e) {
    if(this.checked) {
        correct_answer_selected = true;
        return true;
    }
});

In the event that this.checked is false, your function returns undefined so it is much better to explicitly handle that event.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.