[quote=“TechnoKid, post:21, topic:259656, full:true”]
I have run the code through an HTML validator[/quote]
Which validator was that?
[quote=“TechnoKid, post:21, topic:259656, full:true”]
I then ran the code through a javascript validator and got 51 errors, which I am finding difficult to interpret, let alone resolve![/quote]
I’ll help to give some insight with the javascript validator. I don’t know which one you used, so I’m going to use jslint.com which is one of the more strict ones out there.
Be warned - this reply gets quite long.
Formatting
Most of the code formatting issues can be dealt with by running the code through the Online JavaScript Beautifier.
Needs a “use strict” pragma
Putting the beautified code in to jslint.com, the first issue is with the init function, saying This function needs a "use strict" pragma.
Using strict mode makes it easier to find errors, because it turns some silent errors in to more visible errors, and prevents some inconsistent behavior from occurring.
Typically we place "use strict"
as the first line of the wrapper iife
function:
(function iife() {
"use strict";
// rest of code in here
}());
Instead of doing that, I have placed the “use strict” declaration at the top of the init function, to see if we need anything more after that. If so, another technique can be used instead.
init: function init() {
"use strict";
...
‘remove’ is out of scope
According to jslint.com, functions should appear first before they are used in the code. In this case, because the add()
function calls the remove()
function, that one needs to be moved up to appear before the add()
function.
Undeclared ‘jQuery’
jslint.com needs to know that variables haven’t been misspelled, so we need to tell jslint that jQuery is properly expected. We can do that by placing the following comments at the top of the code:
/*jslint browser */
/*global jQuery */
This function needs a “use strict” pragma
The filterByText() function is outside of the scope of the first "use strict"
that was used. This is a good time to decide if you want to separately add "use strict"
to each needed function, or to use an iife wrapper for all of the code. I’ll go with the second option here, and wrap all of the code with an iife wrapper, allowing me remove all other "use strict"
statements from the code.
/*jslint browser */
/*global jQuery */
(function iife() {
"use strict";
// rest of code here
}());
This does mean though indenting all of the rest of the code, so using the Online JavaScript Beautifier once again helps us to easily achieve that.
Unexpected ‘this’
The this keyword tends to result in far too much confusion, so jslint requires that you use easier to understand techniques than the this keyword.
In this case it is return this.each(...
that is being complained about. Why do we need that? It’s used to handle multiple selectors being called with filterByText, which currently doesn’t exist. It’s also used to assign the this keyword to a variable called select
, which isn’t used either.
We can remove them, and the code continues to work exactly as we require it to.
Undeclared ‘$’
We haven’t told jslint that we are going to use the $ symbol for jQuery.
$(function () {
...
});
The normal way to handle this is to use a jQuery callback, so that the code will be executed when the DOM has finished loading, and also to pass in the $ symbol instead of jQuery:
jQuery(function domReady($) {
// use $ inside here
});
We can replace that iife wrapper with the jQuery one instead, which ends up looking like this:
/*jslint browser */
/*global jQuery */
jQuery(function domReady($) {
"use strict";
// rest of code here
});
Because we now have that domReady wrapper, we no longer need the other wrapper that was used inside of it:
// $(function () {
$('#mydropdown').filterByText($('#mytextbox'), true);
// });
I have commented it here for the sake of example, but the commented lines can be deleted.
Undeclared ‘mydropdown’
The problem line is:
$(mydropdown).find('option').each(function () {
We can now see that the mydropdown
variable hasn’t actually been assigned, and that the earlier variable called select
was most likely supposed to have been mydropdown
instead.
Why did this code work regardless? Because HTML elements with id attributes are given an automatic global variable of the same name, so it was just a coincidence that this worked.
How do we fix this? We can bring back the this keyword, but doing so requires us to tell jslint that we are using the this keyword.
We can also improve things by renaming mytextbox
to searchField, and remove the unused true
from the end of the last line of code.
/*jslint browser, this */
jQuery.fn.filterByText = function (searchField) {
var mydropdown = this;
...
$(searchField).bind('change keyup', function (evt) {
...
...
$('#mydropdown').($('#mytextbox'));
That jslint declaration at the top of the code about the this keyword is a reminder to us that we should investigate removing the this keyword if we can later on.
Use double quotes, not single quotes
For the sake of consistency, only one type of quotes should be used. Because we sometimes use apostrophes in text, and we shouldn’t use HTML code in JavaScript, the double quotes have been chosen to be the single type of quotes that need to be used.
The following is an example of the change that is made to achieve this:
// $(mydropdown).find('option').each(function () {
$(mydropdown).find("option").each(function () {
Redefinition of ‘options’ from line 47
The filterByText()
function uses an options
variable to store all of the available options, and then separately the keyup event retrieves that list of options. The problem here is that the options still exist in the parent scope.
Does this mean that we don’t even need to save aside the options as data at all? Can we do without the code that adds and retrieves the data values?
// $(mydropdown).data('options', options);
$(mydropdown).empty();
// var options = $(mydropdown).empty().data("options");
And everything continues to work as before. There was no need to save information to the data object, when it remains available from the options variable in the filterByText() function.
We can now clean up some of the code that used that data too, by removing the awkward $.each
section and replacing it with a much easier to understand forEach method.
// $.each(options, function (i) {
// var option = options[i];
options.forEach(option) {
...
}
Unused ‘regex’
This code is not used, so it gets deleted.
var regex = new RegExp(search, "gi");
Unused ‘exists’
The exists function isn’t used by anything, and can be removed.
$(document).ready(function () {
// function exists(elemt, arr) {
// return (jQuery.inArray(elemt, arr) > -1);
// }
That document ready line is itself inside of the jQuery wrapper that was placed around all of the code, so that document ready section can be removed too.
// $(document).ready(function () {
functions are inside here, that need to remain
// });
If you are editing the code in a code editor, it is easy to outdent the functions to maintain proper indenting. Because I am doing this via jsfiddle and the jslint console, this is a good time for me to throw the code through the JavaScript beautifier once more.
Unused ‘select’
We can easily remove the unused select
parameter from the function.
// function filterQueueForSelectedOptions(queue, select) {
function filterQueueForSelectedOptions(queue) {
That brings the linting to an end, and we’re left with better code than we started with.
There’s one more thing to fix, and that is the updateResults function near the end of the code. I don’t like how an undefined argument is being passed as the first parameter to the function.
updateResults(selectedOption, selectedOption.parentNode, queue);
...
updateResults(undefined, $("#mydropdown"), queue);
If something is to be undefined, it really should be the last argument to the function. We can instead place the queue first, followed by the dropdown, and have the selectedOption come last.
We can also assign a separate dropdown
variable, to help make things easier to understand.
function updateResults(queue, dropdown, option) {
...
}
...
var selectedOption = evt.target;
var dropdown = selectedOption.parentNode;
updateResults(queue, dropdown, selectedOption);
...
updateResults(queue, $("#mydropdown"));
Looking at the above code now, while it is possible to get the dropdown from the option, it would instead be more consistant to move the dropdown
variable up so that it can be used in both places.
var dropdown = $("#mydropdown");
$("#mydropdown").on("click", "option", function (evt) {
var selectedOption = evt.target;
updateResults(queue, dropdown, selectedOption);
});
$("#mytextbox").on("blur", function () {
updateResults(queue, dropdown);
});
The resulting JavaScript code that we get after all of those changes is found at https://jsfiddle.net/kbq7ymbk/10/
When you let us know which HTML validator you were using before, we can then take a look at things there too.