Web page based on a sample from a book

Question

I've created a web page based on a sample from a book. It works fine, but seems to have become too complicated.

'use strict';
function toggle (triggers, elements, per_trigger, events) {
    function toggle(index, per, elem, func) {
        for (var i = 0; i < per; i++)
            func(elem[index+i]);
    }

    if ((elements.length % triggers.length != 0 && elements.length != triggers.length * per_trigger)
        || (elements.length == 0 || triggers.length == 0)
        || (events[0].func == undefined || events[0].ev == undefined)) {
        console.error('toggle: number of elements must be divisible by' +
        'number of triggers, because one trigger can toggle more than one element\n' +
        'for example: if you want a trigger to toggle two elements, then the per_trigger' +
        ' argument will be 2 and the number of elements must be equal to the number of triggers * 2');
        return false;
    }
    for (var i = 0; i < triggers.length; i++) {
        for (var f = 0; f < events.length; ++f) {
            (function(_i, _f) {
                $(triggers[_i]).on(events[_f].ev, function() {
                    toggle(_i*per_trigger, per_trigger, elements, events[_f].func);
                })
            })(i, f);
        }
    }
    return true;
}

var hidden, trigger, PER_TRIGGER = 1, onclick;
function defineTriggers(hide) {
    hidden = document.getElementsByTagName('div');
    trigger = document.getElementsByTagName('h2');

    if (hide) {
        // hiding elements ..
        for (var r=0; r<hidden.length; ++r) {
            if (hidden[r].displayinfo == undefined)
                hidden[r].displayinfo = hidden[r].style.display;
            hidden[r].style.display = 'none';
            hidden[r].shown = false;
        }
        toggle(trigger, hidden, PER_TRIGGER, [{'ev': 'click', 'func': onclick}]); // defined externally ..
    }
}

function addDiv(date, info) {
    var d = document.createElement('div'),
        s = document.createElement('h2'),
        p = document.createElement('p');
    p.textContent = info;
    s.textContent = date;
    document.body.appendChild(s);
    document.body.appendChild(d);
    d.appendChild(p);
    d.style.display = 'none';
    d.shown = false;
    d.displayinfo = 'block';
    hidden[hidden.length] = d;
    trigger[trigger.length] = s;
    toggle([s], [d], PER_TRIGGER, [{'ev': 'click', 'func' : onclick}]);
}


    alert('running')
    onclick = function(e) {
        e.style.display = e.shown ? 'none' : e.displayinfo;
        e.shown = !e.shown;
    };
    defineTriggers(true);

    // adding btn
    var b = document.createElement('button'),
        f = document.getElementsByTagName('footer')[0];

    f.appendChild(b);
    b.textContent = 'Add a Date'

    b.onclick = function() {
        var day, info, day_regex = /[A-Z][a-z]{1,7} [1-9][0-9]?, 2[0-9]{3}/;
        do {
            day = prompt('Day');
            if (!day_regex.test(day)) alert('Day not correct .. !');
        } while(!day_regex.test(day))
        info = prompt('Info');
        addDiv(day, info);
    }

Is it possible to improve the clarity of this code, without affecting its functionality?

Answer 1

I wrote this up using a lot more jQuery as an example. Confirmed working JSFiddle here.

You can see there's a considerably less lines of code, there's less boilerplate on each line (things like document.getElementsByTagName), and at least in my opinion it's a lot more expressive.

Additionally, it becomes much easier to do other things that jQuery does well. For example, to animate these toggles. Just change $hidden.toggle() to $hidden.slideToggle()

Full code:

'use strict';

function enterData(){
    var day, info, day_regex = /[A-Z][a-z]{1,7} [1-9][0-9]?, 2[0-9]{3}/;
    do {
        day = prompt('Day');
        if (!day_regex.test(day)) alert('Day not correct .. !');
    } while(!day_regex.test(day))
    info = prompt('Info');
    addDiv(day, info);
}

function bindEventsToEl( $trigger, $hidden, events ){

    //This lets you pass in a different hash of events to bind, if you wanted to
    var eventsHash = events || {
        click: function(){
            $hidden.toggle();
        }
    };

    for ( var event_type in eventsHash ){
        $trigger.on( event_type, eventsHash[event_type] );
    }
}


function defineTriggers(hide) {
    var $hidden = $("div"),
        $triggers = $("h2");

    if (hide) {
        $hidden.hide();

        $triggers.each(function(i,el){
            bindEventsToEl( $(el), $( $hidden[i] ) );
        });
    }
}

function addDiv(date, info) {
    //Creating your empty elements and inserting content
    var $header = $("<h2>").text(date),
        $p = $("<p>").text(info),
        $div = $("<div>").append($p).hide();

    //You can chain appends to the root selector
    $("body").append($header).append($div);

    bindEventsToEl( $header, $div );
}


alert('running')

// bind all of our initial header/div combos
defineTriggers(true);

// create the button to add new sections
var $button = $("<button>").text("Add a Date").appendTo("footer").on( "click", enterData );

I removed your PER_TRIGGER, per_trigger variables and the if check around binding event listeners based on their count. It didn't seem to apply in your current JSfiddle anyway, and the reasoning behind it seemed a little shaky.

However, I think this problem might be solved just by using jQuery. Check out the bindEventsToEl function. Because $trigger and $hidden are jQuery selectors, you can have any number of elements in them.

For example, we can call this:

bindEventsToEl( $("h2, div"), $("div") );

This causes ALL divs to toggle whenever ANY h2 or div is clicked. Not exactly useful, but I think it shows you how powerful and expressive jQuery can be when selecting and binding events.

You can pass in a different event hash to the bindEventsToEl as well:

bindEventsToEl( $("h2"), $("div") , {
    click: function(){ console.log("bonus click"); },
    mouseover: function(){ console.log("mouseover"); }
});

If you're still not convinced that jQuery is the route you want to go, some recommendations for your prior code:

• Focus on simplifying your toggle function so that it's much more clear what it's doing.
• Try not to have as much nesting inside functions, break them out into their component parts.
• Definitely don't have another function named 'toggle' inside a function named 'toggle'. It makes it look like it's calling itself recursively.
• Give the rest of your variables slightly clearer names as well.