current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

I'm using JavaScript and JQuery 1.8.3 to rebuild a form built by Django-filter. My purpose is to only present options that will yield a result to the user. The Django stuff all works fine and this is purely a JavaScript question. I should explain that I'm essentially a Python guy and I find JavaScript quite hard to get my head round. My working code looks like this:

<script>
$(document).ready(function() {
    $('#id_scheme').focus();
    $('#id_scheme').change(function() {
        switch (($('#id_scheme').val())) {
            {% for scheme in schemes %}
            case '{{ scheme.pk }}':
            $('#id_paragraph').empty();
            {% for k in scheme.paragraph_set.all %}
            {% if k.decision_set.all %}
            $('#id_paragraph').append('<option value="{{ k.pk }}">{{ k }}</option>');
            {% endif %}
            {% endfor %}
            break;
            {% endfor %}
        }
    });
    $('#id_paragraph').focus(function() {
        switch (($('#id_scheme').val())) {
            {% for scheme in schemes %}
            case '{{ scheme.pk }}':
            $('#id_paragraph').empty();
            {% for k in scheme.paragraph_set.all %}
            {% if k.decision_set.all %}
            $('#id_paragraph').append('<option value="{{ k.pk }}">{{ k }}</option>');
            {% endif %}
            {% endfor %}
            break;
            {% endfor %}
        }
    });
});
</script>

You will see the obvious problem - I'm repeating an anonymous function. I've tried the following methods to make this a named function so I can just call it on both a change to the scheme select and on focussing the paragraph select but with no success.

function myCode() {
    // code you see above here
}
...
$('#id_scheme').change(myCode());

and:

$.myCode = function() {
    // code you see above here
}
...
$('#id_scheme').change($.myCode());

I really hate repeating code. Can anyone tell me what incantation I'm missing?

share|improve this question

closed as off topic by Peter Taylor, Jeff Vanzella, Glenn Rogers, Corbin, Quentin Pradet May 24 '13 at 11:48

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.
    
try ... .change(myCode). –  Jan Dvorak May 23 '13 at 15:36
    
@JanDvorak that's done it. Please post this as an answer and I'll accept. –  cms_mgr May 23 '13 at 15:40
    
This belongs on StackOverflow rather than Code Review. –  Peter Taylor May 23 '13 at 15:48

1 Answer 1

up vote 4 down vote accepted

Your first attempt was good, but: the parentheses in myCode() mean "call the function". You don't want to call it - you want to pass the function as a value. To do this, simply remove the parentheses so that the function is used as a value:

function myCode(){
  ...
}

$('#id_scheme').change(myCode)
$('#id_paragraph').focus(myCode)

Other things to note:

There is no need to prepend id_ to every element ID that you have. This smells of the Types Hungarian notation (which is not particularly useful).

I can see you're generating code. I recommend generating data instead, and keeping the code static. The bonus is that you can use the browser cache to store your code and even without the browser cache, you'll transfer much fewer bytes. I don't know the templating system you're using, but the basic idea is to prepare the data in a format that a JSON encoding library you're using can understand (often such a library understands pretty much everything), and embed the JSON into the code (which parses the JSON):

<script>
   schemes = {{ JSON_encode(schemes) }};
</script>
<script src="main.js"> </script>

main.js:

$(document).ready(function(){
  $('#id_scheme').focus().change(handler);
  $('#id_paragraph').focus(handler);

  function handler() {
    var val = $('#id_scheme').val();
    for(i = 0; i < schemes.length; i++){
      if( val === scheme.pk ){
        ...
      }
    }
  }
}

The other option you've tried ($.myCode = function() {...}) would work as well, but then $.myCode would technically be a jQuery plugin, and it would be visible globally. As far as I can tell, this is not what you want. The only difference here is that myCode, instead of being a local variable, would be a property of the global object $. Again, you made the mistake of calling the function immediately.

share|improve this answer

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