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?

• megawac: Can you provide a jsbin giving an example of the functionality. Also what don't you like about your code now -- anything specific?
• megawac: Also, the first suggestion I have skimming the code is use jQuery more as it appears you have it already included in the page based on the on handler
• Amr Ayman: @megawac: Here .. on JSFiddle
• Amr Ayman: @megawac: I'm not really knowledgeable about jquery, I'm trying to focus on javascript fundamentals at the moment .. That's seems like a good idea, nonetheless .. Can you give me some examples ?
• JaDogg: When you don't add a date and press cancel you go to an endless loop.

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.

• Amr Ayman: I didn't want to use slideToggle because it doesn't affect opacity, so I decided to create one .. Anyways, I've put a link to the final page where I used lots of your code there, thanks ..