Tightening this JS up

JavaScript
1

Hallo all,

I’ve bungled my way through some more JS, and it even (somehow) works on all browsers tested (haven’t tested IE8)! But I see repetition, which tells me I’m writing too much. Also, it’s still a little brittle, but dunno if I can really do anything about that? So, how can I write this better (guess this is mostly a learning question, nothing’s broken… yet)?

This JS basically looks for certain HTML form controls, checks their values on load, and uses that to determine whether a following (usually but not always sibling) stays onscreen or not.

In addition, this is checked again onchange of the form control.

The HTML being affected can be seen here and [url=http://stommepoes.nl/Motorverzekeren/motorbereken1.html]here.

The elements in question have a class of “toggle” (in order to be found), and there are only 3 of these in the first link, only one in the second link.

I have a lib: funclib.js

The main file is here but here also is the current code in this one:


/*motor.js*/

var Motor = {
    init: function () {

        function checkStatus(field){
            var Jong = document.getElementById("Jong"),
                Dekking = document.getElementById("Dekking"),
                Meerkost = document.getElementById('Meerkost');

            function check(control) {
                var val = control.value;
                if (val === 'ja' || val === 'WA-Extra' || val === 'WA-Casco') {
                    Basis.removeClass(field, 'hidden');
                }
                else if (val === 'nee' || val === 'WA') {
                    Basis.addClass(field, 'hidden');
                }
            }

            if (field.id === 'JongField') {
                check(Jong); 
                Jong.onchange = function() {
                    check(this);
                };
            }
            if (field.id === 'Cascodekking') {
                check(Dekking); 
                Dekking.onchange = function() {
                    check(this);
                };
            }
            if (field.id === 'Meerkostja') {
                check(Meerkost); 
                Meerkost.onchange = function() {
                    check(this);
                };
            }
        }

        var hiddenFields = Basis.getElementsByClass('toggle');

        for (i = 0, l = hiddenFields.length; i < l; i++) {
            var field = hiddenFields[i]; 
            checkStatus(field);
        }

    }//init
};

Basis.begin(Motor);

CSS:


form .hidden {
  position: absolute;
  left: -999em;
}

Yeah, you can see I modeled this after the Core namespacing trick but named mine differently because I don’t have nearly the amount of memory management etc that Core has.

I ran this through Lint, fine, but I’m wondering where I can make this better and possibly more flexible. I’m not sure if I’m stuck always listing all the possible values that would equal “true” and all those who may equal “false” (function(check)), or the id’s of the particular elements (since not everyone is always in the same relationship to each other, I can’t just use things like next or prev sibling).

I’m using values rather than selectIndex because the order may be changed arbitrarily. However I also work with a colleague who seems to switch between caps and lowercase in his sleep, so I’ve considered for safety adding some bloataceous toLowerCase() stuff in there, but not sure…

So mostly wondering how I can reduce the repetition here:


            if (field.id === 'JongField') {
                check(Jong); 
                Jong.onchange = function() {
                    check(this);
                };
            }
            if (field.id === 'Cascodekking') {
                check(Dekking); 
                Dekking.onchange = function() {
                    check(this);
                };
            }
            if (field.id === 'Meerkostja') {
                check(Meerkost); 
                Meerkost.onchange = function() {
                    check(this);
                };
            }

And maybe make this value checker better:


            function check(control) {
                var val = control.value;
                if (val === 'ja' || val === 'WA-Extra' || val === 'WA-Casco') {
                    Basis.removeClass(field, 'hidden');
                }
                else if (val === 'nee' || val === 'WA') {
                    Basis.addClass(field, 'hidden');
                }
            }

I keep thinking I should be able to have some list of things that always just mean “true” and another list of things that can just be “false” and have the check function just do one thing if val is true and another if val is false. This may fail in the future if for some reason we needed opposite functionality for true and false values, though.

I went through closure hell trying to get this as far as it is. Often I got errors saying “cannot access optimised closure”. Everything seems to “work” now and passes Lint. Any suggestions? Thanks in advance.

2

You can use a config array here, and for safety sake, keep the creation of the function from outside of the loop.


var config = [
    {'field': 'JongField', 'check': Jong},
    ...
    ],
    i;
function checkField () {
    check(this);
};
for (i = 0; i < config.length; i += 1) {
    if (field.id === config[i].field) {
        check(config[i].check); 
        config[i].field.onchange = checkField;
    }
}

Here you can use Array.indexOf


var values = ['ja', 'WA-Extra', 'WA-Casco'];
if (values.indexOf(val) > -1) {
    Basis.removeClass(field, 'hidden');
}

You know that some web browsers don’t yet know about Array.indexOf (eyeballs IE furiously) so the page link above also includes some compatability code that teaches them how to, until they’re capable of doing it themself.

3

Wow, thanks. Just what I’ve been looking for. I’ve never used forEach (in JS) or Array.indexOf.

The final “i” in config might confuse me, but I’ll see how it works out tomorrow when I’m at work again. And it sounds like a commonly-used name so I can look it up.

4

That final i is to create the i variable for the loop.

You were saying that you use jslint - which ensures that all var statements occur once at the top of the function / code.

5

You were saying that you use jslint - which ensures that all var statements occur once at the top of the function / code.

Yeah initially I had the check() function near the bottom and the for loop near the top (because I wrote it first) and JSLint said I was calling it before it existed or something, so I rearranged everything (before posting here lawlz).

I thought when first looking that the i in config was different than the i in the for loop… because I thought config closed before it. Maybe I picked up a bad habit of indenting children from CSS (my own bad habit, nobody else indents children but me lawlz).

6

Hm, I just tried inching the config array solution in, and found that the .onchange isn’t firing (no errors, and only know that the regular check() is running because some browsers don’t send values back to the default on refresh, so like FF and IE do work every time I refresh but not onchange). I’m not sure if that line’s not getting read or if for some reason the function referenced there isn’t running.
If I’m not getting any actual errors or warnings in the console, what’s the best way to figure out why this isn’t firing? Going back to the three if statements works again, so I know it’s the new check and checkField functions. My array seems good, but I’m still googling 'Javascript “array of object literals” ’ to see what other examples come up.

A related problem I’m having is, Firebug shows me the lib script but not my actual motor.js script. So while I’m not all that familiar with breakpoints, I’ve seen people use debugging tools to watch code play out in slow motion, but I don’t seem to have access to my own script in Firebug.

7

Okay, sitting down and rereading the function for the bazillionth time finally got through my thick head. Function works fine:


            var config = [
                {'field': 'JongField', 'check': Jong},
                {'field': 'Cascodekking', 'check': Dekking},
                {'field': 'Meerkostja', 'check': Meerkost}
                ],
                k; //make sure not messing with other i's
            function checkField () {
              check(this);
            }
            for (k = 0; k < config.length; k += 1) {
                if (field.id === config[k].field) {
                    check(config[k].check);
                    config[k].check.onchange = checkField;
                }
            }

Lawlz, just realised some more brain farts. Why is it that this only happens AFTER I post, no matter how long I wait to do it? : )