Ways to improve a piece of JavaScript

As I gradually learn my way into JavaScript, and perhaps because of the many things I see during the course of putting together the JSWeekly, I’m conscious that my learning materials may not be showing me the best/most current/efficient means of achieving the tasks being set along the way.

With that in mind, I thought I’d take one of the exercises, and see what would be a good way to improve things. The code itself simply iterates through the available window properties and displays them out on screen using the following:

HTML

<div id="main">
  <div id="panel">JavaScript is not enabled/available and this may degrade the viewing experience presented on this webpage</div>
</div>

CSS

body {
  font-size: 1.5em;
}

#main {
  margin: 0 auto;
  max-width: 960px;
}

JavaScript

function init() {
  var panel = document.getElementById("panel");
  var property;
  panel.innerHTML = "";
  for (property in window) {
    if (property) {
      panel.innerHTML += property + ", ";
    }
  }
}
document.addEventListener("DOMContentLoaded", init, false);

You can see the working version here on Codepen

Now to see what I could do without too much thinking, I’ve wrapped it all up in an IIFE, added 'Use Strict'; to it, and as it’s now an IIFE, I figured that I didn’t need the event listener any more, so just made a straight call to init(). Lastly, I moved the JS call from the <head> section to just before the </body> tag.

So far so good and it all works, but what else can I do with it that might ‘improve’ things? Is there anything I could consider from ES2015 for example? Or is this example too simple to be worth playing around any further on. All thoughts welcomed.

I forked the original pen, so you can see what it now looks like here

3 Likes

Just replace the init function with an IIFE, and you don’t have to call anything at all…

(function() {
  var panel = document.getElementById("panel");

  panel.innerHTML = "";

  for (var property in window) {
    panel.innerHTML += property + ", ";
  }
})();

But be cautious regarding the event listener – you don’t need it here because the IIFE is at the bottom of the page, not because the code is inside an IIFE per se.

2 Likes

Funnily enough, that occurred to me as I was brushing my teeth before getting into bed just now.

The bit about the IIFE location being the trigger, rather than its mere existence, I didn’t know, so thanks for that.

For elements you only want to have displayed if the required level of JavaScript is not available its more easily done using CSS and one line of JavaScript in the head to add a class to the <html> tag.

That allows you to both specify what JavaScript commands the browser needs to support for you to consider the browser to support JavaScript AND to hide both block and inline elements all at the same time.

I see that you’re using:

if (property) {

The standard method when using for…in is to check against the hasOwnProperty method of the parent, such as:

for (property in window) {
    if (window.hasOwnProperty(property)) {
        ...
    }
}

and if you’re going to be doing that, you might as well do it more efficiently, replacing it with:

Object.keys(window).forEach(function (property) {
    ...
});

We can now make things even simpler, by using join on the array of keys.

var panel = document.getElementById("panel");
panel.innerHTML = Object.keys(window).join(", ");

Is that enough of an improvement?

3 Likes

Is it worth using just a one-liner?

document.querySelector("#panel").innerHTML = Object.keys(window).join(", ");
2 Likes

Thanks @felgall and @Paul_Wilkins - I’m a bit tied up right know to digest what you’ve said fully, but I will be back a little later.

Am I right in thinking your comment relates to this line of HTML <div id="panel">JavaScript is not enabled/available and this may degrade the viewing experience presented on this webpage</div>, and the JS that blanks it ahead of being re-populated?

I guess you could more accurately say I’m copying, but yes… :wink:

It’s definitely the kind of thing I’m getting at. Clearly the book is aimed at beginners and is trying to ensure you piece together the basics in a structured form, that can be built upon. In a sense, I’m trying to get slightly ahead of myself, on the basis that I can already recognise that there are better ways of doing the job. As you’ve introduced me to a few methods/functions/objects I’ve not seen before, I need to just play around with them to be sure I understand what’s going on. Your one-liner does offer a much reduced version of where I started from though, and was the kind of thing I thought would be possible.

yes.

One line of JavaScript and some CSS can hide as much content in the page as you like based on the level of JavaScript support the browser has.

I do feel that I should point out though, that even though a one-liner is possible, it’s more important for the code to be clear and easily understandable for anyone that’s coming across the code.

A second concern too is also ensuring that any future changes are easy to apply, which is what helps to encourage some of these code structures.

1 Like

I’d very much agree with the point on being clear and understandable. For this particular example, it feels to me that even the one-liner should be relatively understandable with a little experience. I can well imagine it becomes harder to digest as it’s used within something of rather greater scale.

The point about future changes is perhaps some distance away from me yet, so I’d need to give it more thought - not that I disagree with the principle, just that I’d perhaps need a better example to see what that means in practice.

With functions, there tend to be three distinct stages to them.

  1. Prepare the variables.
  2. Do something.
  3. Return or affect something.

Sometimes there is a preliminary guard-clause, but that’s the main structure of each function.

If you stay good with that, you won’t go far wrong.

Let’s do this as an example.

###Prepare the variables

What are we going to call the function and what arguments will it have? We have some options on how we might want to use it.

  1. panel.innerHTML = getProperties(window).join(', ');
  2. panel.innerHTML = showProperties(window);
  3. showProperties(window, panel);
  4. showArray(getProperties(window), panel);

All are valid, but the third one seems to make it easiest to use the function. With the understanding that function has two different tasks to perform. This should mean using two functions. One called getProperties, and the other called showProperties that uses the first function.

Our showProperties function has two main pieces of information that can change. The object that we want to see the properties of, and the target element within which to show that information.

What should the target be? Should it be a string value of an identifier, should it be a selector of some kind, or should it be an element itself?

  1. showProperties(window, “panel”);
  2. showProperties(window, “#panel”);
  3. showProperties(window, panel);

Let’s keep things simple by passing the actual element as the target.

Because the main purpose of the showProperties function is to show the element properties, I don’t think that it’s appropriate for the function to also focus its time on finding the properties, so we should use a separate function called getProperties to get them, thus simplifying what the functions need to deal with.

function getProperties(obj) {
    ...
}
function showProperties(obj, el) {
    ...
}

###Do something
The getProperties function can be handled in any number of ways. You could use Object.keys, but for maximum compatibility you can use the for…in technique.

We can use the same prepare/do/return structure with the getProperties function.

function getProperties(obj) {
    // prepare
    var prop = "",
        props = [];
    // do something
    for (prop in obj) {
        if (obj.hasOwnProperty) {
            props[props.length] = prop;
        }
    }
    // return
    return props;
}

If we’re sure that the computers involved can all handle ES5, we can simplify this getProperties function to instead be:

function getProperties(obj) {
    return Object.keys(obj);
}

###Return or affect something
The showProperties function is just going to get the array, and display it on the given element.

showProperties(obj, el) {
    var props = getProperties(obj);
    el.innerHTML = props.join(", ");
}

We can place these functions inside an IIFE too, so that the complexities of what’s being done are hidden behind it.

With the IIFE we can from inside, return only the showProperties function, which becomes the public access to what’s being done.

var showProps = (function () {
    // functions here

    return showProperties();
}());

The showProperties function will have access to other functions within the IIFE. Nothing else outside is allowed to know about them.

###The end result

In total this will end up being:

var showProps = (function () {
    function getProperties(obj) {
        return Object.keys(obj);
    }
    showProperties(obj, el) {
        var props = getProperties(obj);
        el.innerHTML = props.join(", ");
    }
    return showProperties();
}());

showProps(window, document.querySelector('#panel'));

The names showProps and showProperties don’t have to be different. I’ve just made them separate here for the sake of clarity.

When is it appropriate to use a variable instead of a direct reference such as querySelector? The general rule is that when used once it’s okay. Twice, consider using a variable. Three times, definately use a variable.

###Other considerations
Now that we have the IIFE structured code, we can consider making things simpler. We can get rid of the IIFE. If we don’t want a couple of different functions floating around, and we don’t intend for getProperties to be used by anything else, we can move the getProperties function inside of showProperties.

function showProperties(obj, el) {
    function getProperties(obj) {
        return Object.keys(obj);
    }

    var props = getProperties(obj);
    el.innerHTML = props.join(", ");
}

showProperties(window, document.querySelector('#panel'));

Is that better? It seems to be, until other complexities require us to use an IIFE once again.

The last improvement is to use a panel variable, to be consistent with good practice, and help simplify how the showProps function is used.

function showProperties(obj, el) {
    function getProperties(obj) {
        return Object.keys(obj);
    }

    var props = getProperties(obj);
    el.innerHTML = props.join(", ");
}

var panel = document.querySelector('#panel');
showProperties(window, panel);

It’s always a balancing act of needs and requirements that’s involved here.
Where we’re at right now looks to be a good balance of things at this stage.

3 Likes

I’ve barely skimmed this so far, but it looks like it could happily sit as an article rather than a forum post - time to read…

I have taken it into a different direction, see if JS is loaded:

JavaScript is not enabled/available and this may degrade your experience on this site.
<script>
	function init()
	{
		var testDiv = document.getElementById("JStest");
		testDiv.innerHTML = "JavaScript is active. Enjoy your time here!";
		var workDiv = document.getElementById("JSenabled");
		workDiv.innerHTML = "<br><br><br><h1>All is well, let's get on with it!!!</h1>";
	}
	document.addEventListener("DOMContentLoaded", init, false);
</script>

Seems I cannot quote html…

Simplest way to do that is:

<style type="text/css">
.js .noscript {display:none;}
</style>
<script>
if (window.addEventListener) document.documentElement.className += ' js';;
</script>
</head>
<body>
<p class="noscript">JavaScript is not enabled/available and this may degrade your experience on this site.
</p>
<div id="JStest"></div>

<script>
(function() {
'use strict';
if (!window.addEventListener) return;
var testDiv = document.getElementById("JStest");
testDiv.innerHTML = "JavaScript is active. Enjoy your time here!";
}());
</script>
</body>

This has several benefits:
1 you can hide everything you want to hide in the page all in one go by giving it a class="noscript"
2. You can determine the level of JavaScript support the script requires and treat older browsers as if they don’t support any JavaScript (in this example IE8 doesn’t support addEventListener so the noscript version of the page is what they see.
3. The script itself will not crash when it is run on a browser that doesn’t support all the JavaScript commands the script uses. (although your otherwise unnecessary DOMContentLoaded event does serve that purpose as a side effect in your code)

The extra code this alternative uses to handle one paragraph to be hidden becomes significantly less code than the alternative when there are more pieces of thest to be hidden as this code does all of them no matter how many there are with only the class needing to be set on each extra piece of text.

Note that you would normally have the CSS and the bottom script in separate files - I included it all together so you can see how the different pieces work.

I just popped back to ask how you did it, only to find you’d already done so. Thanks.

I got it working, though I needed to modify the second piece of JS slightly (curly brackets around the return statement)

(function() {
  'use strict';
  if (!window.addEventListener) {
    return;
  }
  var testDiv = document.getElementById('JStest');
  testDiv.innerHTML = 'JavaScript is active. Enjoy your time here!';
}());

At the moment, I’m not too sure the piece of JS in the <head> is working though. I’ll keep poking at it…

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.