The link you posted is broken. Just to be complete: http://ejohn.org/blog/getelementsbyclassname-pre-prototype-16/
This is because of hoisting, right? Would it be correct to say that the above example gets interpreted like this:
var foo = function(){
bar();
},
bar; // bar undefined
foo();
bar = function () {
...
} // bar now has a value
That makes sense now.
Thank you!
Yes, that’s right. It’s for similar reasons that an object cannot call one of its own methods when it’s being created.
var foo = {
someMethod: function () {
return 4;
},
someProperty: foo.someMethod() // fails
};
Because the foo object at that time is not actually an object - it is still undefined at the time that the object is being defined.
The next part to deal with is in terms of the idx variable, and the template side of things.
These two lines can be removed.
template.id = "container" + idx;
template.className = "container";
The code that changes the id of the template isn’t actually needed. All that’s needed is to remove the id from the template part itself.
template.removeAttribute('id');
And for the className part, it’s more appropriate for that to be on the template itself. So the template can have a class of “lottotemplate” that we will use, and the container can have a class of “lottocontainer”.
Moving away from unique identifiers helps us to make the code more flexible so that it can be used in multiple places on the same page, if desired.
We can help to keep the code simple too by using document.querySelector(), which works well down to IE8. We can achieve IE7 and some earlier by using a querySelector polyfill, one of which can be found in http://www.calormen.com/polyfill/polyfill.js
<div class="lottocontainer">...</div>
<div class="lottotemplate">...</div>
I want to call it with something like the following:
lotto({
container: document.querySelector('.lottocontainer'),
template: document.querySelector('.lottotemplate')
});
which means making lotto a global object. And hopefully that will be the only global object that the code has.
So take the existing lotto function:
var lotto = function (options) {
...
},
form = ...;
and turn it in to a global object
window.lotto = function (options) {
...
};
var form = ...;
We can’t call lotto yet though with our desired container and template information. Not before some cleaning up has occurred so that we don’t end up making lots of in-code calls to, for example, opts.container
What we need to do is to move the remaining code within the lotto function to their own functions, so that parameterised calls can be easily made to them.
Before we do that though, we should remove the direct assignment of an element to the container and template variables, and just use a classname selector there for them instead.
We currently have:
container = document.querySelector('.container'),
template = document.getElementById('template').cloneNode(true),
playButton = template.getElementsByTagName("input")[0],
resultDiv = template.getElementsByTagName("div")[0];
...
template.removeAttribute('id');
container.innerHTML = '';
container.appendChild(template);
Let’s replace the template with a classname selector instead. That will help us to prepare for passing that info in to the lotto function, and we can move that cloneNode to a better place too.
<div class="lotto template">
...
</div>
.template {
display:none;
}
templateSelector = '.lotto.template',
...;
// and further down below the roll function
var template = document.querySelector(templateSelector).cloneNode(true);
The HTML and the CSS are a lot better there now. Because we are now using a selector to get the template, the playButton and resultDiv variables can also be replaced by selectors, with the playButton and resultDiv variables being moved down to where the template is dealt with too.
Also, once the template has been cloned, it really isn’t template content anymore. So this could be a good time too now to change things from template to content.
container = document.querySelector('.container'),
templateSelector = '.lotto.template',
playSelector = 'input',
resultSelector = 'div';
...
var content = document.querySelector(templateSelector).cloneNode(true),
playButton = content.querySelector(playSelector),
resultDiv = content.querySelector(resultSelector);
removeClass(content, 'template');
...
container.innerHTML = '';
container.appendChild(content);
Thanks to the addition of a removeClass function from http://snipplr.com/view/3561/ we now have selectors for the template and its parts, and the template classname is neatly removed from the content that goes in to the container.
Here’s what we have at this stage. http://jsfiddle.net/pmw57/8R42e/20/
Tomorrow we should be able to use a similar selector for the container, and move the code below the roll function in to its own separate function for easy handling.
That should help us to easily call the lotto function with our own custom selectors for the container and template, instead of needing to have them embedded within the lotto code.
Good stuff, Paul!
I have two questions:
Why do we have to make lotto a global?
Are in-code calls really that much of a no no?
What would be the alternative?
Also, it’s nice to see you favour concise code (such as document.querySelector()) even if it means pollyfilling the functionality for older browsers. It’s always tempting not to do this and take the easy path instead (or worse still, throw jQuery at a problem).
Because that’s the only effective way to use it in different types of situations on a page, or with differently named areas for the container and template.
We might have the template as being optional, so that the container is what is used for the template.
We might have the container auto-init itself if an appropriately named element exists.
But to do anything with it after the page has loaded, means having it available in some manner, and that is via a globally accessible object. In much the same way that jQuery is globally accessible via the $ symbol.
It’s not so much of a no-no, but more of a less flexibility in how the code can be used.
We can now fix up the container side of things, which currently starts out as:
container = document.getElementById('container'),
templateSelector = '.lotto.template',
playSelector = 'input',
resultSelector = 'div';
...
var content = document.querySelector(templateSelector).cloneNode(true),
...
container.innerHTML = '';
container.appendChild(content);
We can use the same selector technique for the container. While we are here, I’ll also make the play and result selectors a bit more specific too
<input type="button" class="play">
<div class="result"></div>
containerSelector = '.lotto.container',
templateSelector = '.lotto.template',
playSelector = '.play',
resultSelector = '.result';
...
var container = document.querySelector(containerSelector),
content = ...
What we want to do is to move these selectors in to the opts object, but before we do that, we should first refactor the below code in to separate functions.
var container = document.querySelector(containerSelector),
content = document.querySelector(templateSelector).cloneNode(true),
playButton = content.querySelector(playSelector),
resultDiv = content.querySelector(resultSelector);
removeClass(content, 'template');
playButton.id = "play" + idx;
playButton.value = opts.buttonText;
playButton.onclick = function justOnce() {
this.blur();
if (counter > 0) {
return false;
}
roll();
};
resultDiv.id = "result" + idx;
resultDiv.innerHTML = opts.initialText;
container.innerHTML = '';
container.appendChild(content);
What do we have there, is code to setup the content from that template, and to update the container with that content.
That’s a lot of code for just one function, due to several responsibilities being involved, so what I want to do instead is to call an init function to get things going. That init function can then pass the opts info to where it’s needed.
init(opts) {
var container = document.querySelector(opts.containerSelector),
template = document.querySelector(opts.containerSelector),
content = generateContentFromTemplate(template, opts);
updateContainer(container, content);
}
That’s how I want things to be structured.
With the generateContentFromTemplate() function we can currently move the existing code in to that function.
function generateContentFromTemplate(template, opts) {
var content = document.querySelector(selector).cloneNode(true),
playButton = content.querySelector(playSelector),
resultDiv = content.querySelector(resultSelector);
removeClass(content, 'template');
playButton.id = "play" + idx;
playButton.value = opts.buttonText;
playButton.onclick = function justOnce() {
this.blur();
if (counter > 0) {
return false;
}
roll();
};
resultDiv.id = "result" + idx;
resultDiv.innerHTML = opts.initialText;
return content;
}
...
content = generateContentFromTemplate(templateSelector, opts);
We will come back to this later on, after the restructuring has occurred.
After moving that code in to the function, the roll function can’t find the resultDiv variable. Because two different places want to update the resultDiv, this is a good time to create a function called updateResult just for that purpose, where we can use the selector in opts.resultSelector to specify where the update occurs.
Because the result that we want to update may be on the page, or it may be in the template content that we are working with, we will want to pass a context to the updateResult function too. We can also default that to the document object if none is provided.
function updateResult(html, context) {
context = context || document;
var resultSelector = opts.resultSelector,
result = context.querySelector(resultSelector);
result.innerHTML = html;
}
...
var draw = ...,
container = document.querySelector(containerSelector);
...
updateResult(draw.join(' '), container);
and the following line from generateContentFromTemplate() can be replaced too:
[color="red"][s]resultDiv = content.querySelector(opts.resultSelector);[/s][/color]
...
[color="red"][s]resultDiv.innerHTML = opts.initialText;[/s][/color]
[color="green"]updateResult(opts.initialText, content);[/color]
Things are now ready for us to move the selectors in to the opts object.
We can also get the selector first from the options passed to the lotto function, before defaulting to our default selectors. That way they can be overridden when calling the lotto function.
We do though need to protect ourself from when lotto is called with no options.
if (!options) {
options = {};
}
var opts = {
pick: options.pick || 6,
from: options.from || 1,
to: options.to || 49,
containerSelector: options.containerSelector || '.lotto.container',
templateSelector: options.templateSelector || '.lotto.template',
playSelector: options.playSelector || '.play',
resultSelector: options.resultSelector || '.result',
totalRolls: options.totalRolls || 50,
rollDelay: options.rollDelay || 50,
buttonText: options.buttonText || "Lotto Lucky Dip",
initialText: options.initialText || "Your Lucky Numbers"
},
They are listed in an approximate order of importance, but it’s entirely up to you. You can list them, for example, in alphabetical order instead of yolu like.
The move to the opts function means that some pieces of code now need to be updated, to retrieve the information from there.
Thanks to the restructuring though, it’s not many pieces at all though.
var result = context.querySelector(opts.resultSelector);
...
container = document.querySelector(opts.containerSelector);
...
playButton = content.querySelector(opts.playSelector),
resultDiv = content.querySelector(opts.resultSelector);
...
var container = document.querySelector(opts.containerSelector),
template = document.querySelector(opts.templateSelector);
Another change that I’ll want to make is to have the defaults specified separately, and to then loop through them adding them to any missing properties from the options, but that’s for later on the todo list.
Let us now get back to that generateContentFromTemplate function. Here’s the current code that we’ll be working on.
function generateContentFromTemplate(template, opts) {
var content = template.cloneNode(true),
playButton = content.querySelector(opts.playSelector),
resultDiv = content.querySelector(opts.resultSelector);
removeClass(content, 'template');
playButton.id = "play" + idx;
playButton.value = opts.buttonText;
playButton.onclick = function justOnce() {
this.blur();
if (counter > 0) {
return false;
}
roll();
};
resultDiv.id = "result" + idx;
updateResult(opts.initialText, content);
return content;
}
Here’s what needs to be done with it:
[list][]the idx parts aren’t needed at all
[]the play section could do with being extracted out to its own function
[*]I’m not happy with that removeClass section[/list]
Removing the idx parts is easy, and the variable declaration further up for is can be removed too.
[color="red"][s]idx = document.getElementsByTagName('div').length,[/s][/color]
...
[color="red"][s]playButton.id = "play" + idx;
resultDiv.id = "result" + idx;[/s][/color]
The playButton code can be moved in to its own function, and called from within the generateContentFromTemplate one:
function setupPlayButton(button, value) {
button.value = value;
button.onclick = function rollLottoNumbers() {
this.blur();
if (counter > 0) {
return false;
}
roll();
};
}
...
setupPlayButton(playButton, opts.buttonText);
With the removeClass part, not only does it seem like a needless detail but it also results in the following nested div HTML structure for the container:
<div class="lotto container">
<div class="lotto">
<!-- template content in here -->
</div>
</div>
What would be good instead, is to remove that inner div - to unwrap it from the content.
Let’s move the problem out to a separate function, so that we can clean up the generateContentFromTemplate() function and focus on this last remaining problem.
function generateContentFromTemplate(template, opts) {
var content = getTemplateContent(template),
playButton = content.querySelector(opts.playSelector),
resultDiv = content.querySelector(opts.resultSelector);
setupPlayButton(playButton, opts.buttonText);
updateResult(opts.initialText, content);
return content;
}
That function is now looking a lot better than it was.
The getTemplateContent() function starts off like this:
function getTemplateContent(template) {
var content = template.cloneNode(true);
removeClass(content, 'template');
return content;
}
What I want to do here is to create a document fragment, and copy the child nodes of the template over to there instead.
function getTemplateContent(template) {
var copyOfTemplate = template.cloneNode(true),
content = document.createDocumentFragment();
while (copyOfTemplate.hasChildNodes()) {
content.appendChild(copyOfTemplate.firstChild);
}
return content;
}
And now the class-handling functions, such as removeClass that we had earlier, can now be removed.
Lastly for now, the form-handling code should be moved out of the wrapper for the lotto code, and placed at the end below the wrapper. This allows the lotto code to be easily moved out to a separate javascript file.
So now the lotto code can be started with just a call to lotto(), or other options can be given to it to change how it behaves.
JSLint is still happy with all of the code, when we tell it to assume that we’re using a browser, and there isn’t much more with the refactoring left to be done.
The code that we now have is found at http://jsfiddle.net/pmw57/8R42e/22/
I hope you can agree that this refactoring has been worth it.
[ot]Mad props to the SitePoint developers. :drink:
I foolishly performed a web browser update while the preview for this post was left in an open tab, and I was able to retrieve ALL of this super-long post using the auto-restore content link.
It seriously helps to alleviate a bad situation for me. :headbang:[/ot]
So, I just finished working through all of that (Phew!)
This has really been a good learning experience in making code more readable and therefore by definition easier to maintain.
I hadn’t expected such an in-depth response when I posted my original question.
Thank you!
There was one last thing I wanted to ask: if we want multiple containers, how would we differentiate between them?
Like this?
<div class="lotto container"></div>
<div class="lotto container2"></div>
<div class="lotto container1"></div>
etc...
No, adding numbers to the class name is a bad way to do that.
The next stage of things that I’ll take with the code, is how to make it work in multiple situations. Even if they seem to be identical.
I’ll get on to that tomorrow.
Sorry, I didn’t want to cause you more work 
It was just that we discussed this briefly beforehand and this was the only way I could see to currently do it (or with an unique identifier).
Also, what are your thoughts on making this code work with Handlebars (as Jeff Mott’s suggestion in post#5)?
I was just playing around with Handlebars and got it working ok, but couldn’t see any added bonus it would bring in the light of how we currently have things set up.
What we has earlier wasn’t as capable of being worked with what we now have, so it’s a good opportunity to delve in to what needs to be done to allow such things to be worked on in a more flexible manner.
I agree. It’s useful for larger projects, but overkill for what we’re working with here.
The next level of detail is to allow the code to run in multiple instances on the page. To achieve this, we cannot have the code always query the page for the section that it’s working with. So we want the code to make only one query when it’s being used, and to then refer to a stored reference of that query on future occasions.
Currently there are 4 queries when the lucky dip starts. Two are for the container and the template, which are reasonable, and the other two are of the template for the play button and the result area, which are reasonable too.
When the play button is pressed though, every single update of the numbers as they roll results in a search of the document for the container and for the result area, which is not reasonable. Those searches all need to be fixed so that rolling doesn’t result in any searches at all.
An effective way of doing that is to assign the container to the opts object, and retrieve that reference from the roll function.
var opts = {
[color="green"]container: opts.container[/color],
...
};
[color="green"]opts.container = opts.container || document.querySelector(opts.containerSelector);[/color]
...
function roll() {
...
container = [color="green"]opts.container[/color][color="red"][s]document.querySelector(opts.containerSelector)[/s][/color];
...
In fact, it could be useful to have all of the selectors resolved in the opts object, or to at least have a property there that is waiting for a reference to be assigned:
var opts = {
container: options.container,
template: options.template,
...
};
opts.container = opts.container || document.querySelector(opts.containerSelector);
opts.template = opts.template || document.querySelector(opts.templateSelector);
opts.playButton = {};
opts.resultArea = {};
So a container or a template can be provided, and if they aren’t then they will be assigned from the selector instead. For example:
lotto({
container: document.querySelector('#lotto1'),
pick: 4,
from: 1,
to: 40
});
We can now make use of opts.container and opts.template in the init() function, where we can simplify the request for the container and template:
var container = [color="green"]opts.container[/color][color="red"][s]document.querySelector(opts.containerSelector)[/s][/color],
template = [color="green"]opts.template[/color][color="red"][s]document.querySelector(opts.templateSelector)[/s][/color],
content = generateContentFromTemplate(template, opts);
and in the generateContentFromTemplate() function we can assign the play button and result area to the opts object, so that we may make good use of them later on:
function generateContentFromTemplate(template, opts) {
...
opts.playButton = content.querySelector(opts.playSelector);
opts.resultArea = content.querySelector(opts.resultSelector);
setupPlayButton(opts.playButton, opts.buttonText);
updateResult(opts.initialText, template);
...
So now that we have the result area defined, we don’t need to pass a context to the updateResult() function anymore. We can instead make the resultArea() function a lot simpler now.
function updateResult(html, resultArea) {
resultArea.innerHTML = html;
}
...
//updateResult(draw.join(' '), [color="green"]container[/color][color="red"][s]opts.resultArea[/s][/color]);
...
//updateResult(draw.join(' '), [color="green"]template[/color][color="red"][s]opts.resultArea[/s][/color]);
And now the lucky dip can run in multiple different instances, without them interfering with each other.
As a last bit for now, I want to just have the following HTML element that can then contain the lotto object:
<div id="lotto2"></div>
And to achieve that, I’ll want to add the “lotto container” class names to the container. Previously I would have been tempted to do that manually, or use an addClass function, but there is now element.classList which we can use to get a list of classes on an object, and add/remove them too.
So we can add the following to the init() function:
container.classList.add('lotto');
container.classList.add('container');
and the following is all that’s now needed to create a new lucky dip in a different location:
lotto({
container: document.querySelector('#lotto2')
});
Or even just:
lotto({
containerSelector: '#lotto2'
});
The test code with the above changes can be found at http://jsfiddle.net/pmw57/8R42e/23/
The next thing on the TODO list is to rework the code in to a module pattern, which helps to give it a more well-known structure that can be easily maintained and extended.
To do so using TDD to create unit tests, and perhaps intern might be an interesting way to explore such techniques.
Hi,
It took a while to remember where we’d got to with this, but looking back through the old fiddles helped.
I like the changes you made, preventing the roll() function from querying the DOM was an easy win.
I do however have two questions:
javascript opts.playButton = {}; opts.resultArea = {}; javascript opts.playButton = content.querySelector(opts.playSelector); opts.resultArea = content.querySelector(opts.resultSelector); That would be good.
That would be even better and something I would like to learn more about.
Mostly it was to keep the variable assignments for all elements in just one place.
It also helps though with allowing them to be accessed from other parts of the script too. It doesn’t need to occur though, and if we later on have a user-supplied function to set up the template, they should be removed.
Sometimes you won’t want to give the user the ability to change those settings. This lucky dip is capable of working in bot situations. A form to provide such controls is already here for one of them, so a demo of other uses without the form is also given too.
Yes, that can be easily dealt with by attaching the code and accessor functions on to the container itself. After getting some tests in place, we can make that sort of change and use the tests to ensure that things continue to work in a proper manner.