I just went to the Samsung Developer Conference in San Francisco and one thing in the goodie-bag was a book (paper, no less) on building apps for the amazing new Smart TVs they are bringing out. Exciting stuff, especially as the platform is HTML5 and JavaScript driven. When opening the book randomly though I found the following code:
Click for bigger
<div id="left">
<a href="javascript:void(0);" id="left_anchor"
onkeydown="left.keyDown();">left</a>
</div>
<div id="right">
<a href="javascript:void(0);" id="right_anchor"
onkeydown="right.keyDown();">right</a>
</div> |
<div id="left">
<a href="javascript:void(0);" id="left_anchor"
onkeydown="left.keyDown();">left</a>
</div>
<div id="right">
<a href="javascript:void(0);" id="right_anchor"
onkeydown="right.keyDown();">right</a>
</div>
// left anchor event handling function
left.keyDown = function ()
{
var keyCode = event.keyCode;
switch (keyCode)
{
case tvKey.KEY_RIGHT:
jQuery('#right_anchor').focus();
jQuery('#right').addClass('focus');
jQuery('#left').removeClass('focus');
break;
default:
break;
}
}; |
// left anchor event handling function
left.keyDown = function ()
{
var keyCode = event.keyCode;
switch (keyCode)
{
case tvKey.KEY_RIGHT:
jQuery('#right_anchor').focus();
jQuery('#right').addClass('focus');
jQuery('#left').removeClass('focus');
break;
default:
break;
}
};
I do understand that a SmartTV is a defined environment and there might be things that work better when you do things in a certain way, but I am really hard pressed to see any reason why anyone in 2013 would write JavaScript and HTML like that unless what happened is that we keep copying old code without thinking of the impact it has.
There is a huge movement of teaching people to code. Great startups like Codecademy, Codecombat, Codewars and many, many more use the web to teach people how to build things for the web, how to grasp the concepts and logic of code and how to write it. On the other side of the coin we have implementation guides that ride roughshod over everything we learned in all the recent years we used web technologies for the sake of writing a very “easy to understand” hello world example or “keep the code short”. This is not helping, all it does is perpetuate the prejudice that the web is not a professional environment to work in and instead it is OK just to copy and paste something and change some function names as long as the browser understands it.
We got into a world of copying and pasting solutions that work without caring at all about the effects they might have. This is unfortunate, seeing how JavaScript has become a first class language, or as Shinetech put it in their excellent “Respect the JavaScript” post:
It is the only runtime language I can think of that will run on any modern processing device. It’s ubiquitous and the ecosystem has evolved beyond what anyone could have imagined, with many implementations (Rhino, V8, now Nashorn) and it’s very own matching server-side equal, node.js
So, what is so terrible about this code? Chris, why are you such an arrogant bastard in your web standards ivory tower punishing people who just want to write code and release products quickly?
Call me old fashioned, but I don’t like waste. I also don’t like when people use things without understanding the consequences. Our goal should not be “it renders, therefore it must be right”, instead it should be “this is clean, extensible and it uses various technologies benefiting of their different strengths”.
Use the wrong element, then make it work?
So what’s wrong here? The first glaring, utterly useless and bloated mistake that does not seem to want to die is links with a href attribute of javascript:void(0) (I lamented about this in 2005 in Six JavaScript features we do not need any longer).
A link is there to point to a resource. That’s what we defined them for. If you don’t point to a resource – don’t use a link. Much like you don’t use an H1 when you want to define an INPUT, although modern JavaScript and CSS are perfectly capable of making one act like the other. It does not make sense though, which is why you don’t do it. So why use links with href attributes that are long and – by definition – don’t do anything?
If you want something that is focusable, can start a script when the user interacts with it and is both keyboard and mouse accessible, use a BUTTON element. This is what they are for.
Add an ID, then you can access it easily?
The next problem is the use of IDs on the parent element and on each button. It is simple to define a button with an ID and make it styleable and accessible either with getElementById() or querySelector() but it also means that this is a unique thing in the whole document. What if you want more than one button that enables navigation to the right? You need to add a different ID and check for that one in the code. This is wasteful and limits the extensibility of your interface.
Using IDs is simple, but it means that you create a unique element with unique functionality – are you sure you won’t need to repeat the same element at a later stage?
Everybody in line
The next massive issue is the inline event handler on every single element. This seems to be special to the SmartTV platform – the API emits a key event that tells you what key was pressed on the remote control. In essence, all you would need to do is to assign one key event handler to the whole document instead of lots of inline handlers to every single control. This is not only wasteful and mixes HTML and JavaScript (remember, we do work in a pure JavaScript environment here), it also means that you are very likely to repeat the same code over and over again for each control rather than writing a single handleKeys() function that can be activated by various controls depending on what view of your application you are.
If you use an inline event handler on an HTML element you make debugging harder, you make it much harder to refactor code (as function names need to change in your JS and in each HTML documents) and you are very likely to write code that repeats the same functionality in different listeners.
This actually happens in this example as on the next page the right.keyDown method is a copy of left.keyDown with the only difference being the shifting of focus and class names.
Goldfish code
The next bit that is worrying here are all the jQuery calls to get references to elements to set the focus and to remove and add classes to. First of all, hey, we have a jQuery dependency here, why didn’t the rest of the code use jQuery for the event handling? Secondly, why all the repetition? Every time you access the DOM you slow down your app. We know these elements, we gave them IDs to ensure that we know them. That’s why asking jQuery to find them again for us every single time an event is dispatched means your code has the memory capacity of a goldfish (and uses a lot more memory). If you can, always cache elements in variables to avoid yet another DOM lookup. Simple caching like this goes a long way:
/* Navigation buttons */
var navright = document.querySelector('#right');
var navleft = document.querySelector('#left');
var navup = document.querySelector('#up');
var navdown = document.querySelector('#down');
// from now on use navright, navleft, navup, navdown
// this also means you can rename the IDs later without
// having to hunt all your code for them! |
/* Navigation buttons */
var navright = document.querySelector('#right');
var navleft = document.querySelector('#left');
var navup = document.querySelector('#up');
var navdown = document.querySelector('#down');
// from now on use navright, navleft, navup, navdown
// this also means you can rename the IDs later without
// having to hunt all your code for them!
Unnecessary shifting of classes
The last obvious issue here is the shifting of classes from the left to the right. The problem with this is that if you were to add more buttons you’d have to add them to the method as you right now do not know which one was activated. This means you need to either add a “remove class from x” line for each button or write a loop over all buttons removing the class before setting it to the current one. This is a very common and simple to rectify mistake.
First of all, there is no need to set a class to an element if you want to highlight it when it is focused. Setting an active and focus style in CSS is enough (as shown here).
button {
display: block;
}
button:active {
background: green;
}
button:focus {
background: green;
} |
button {
display: block;
}
button:active {
background: green;
}
button:focus {
background: green;
}
But even if you need to set a class to the currently focused element, there is no need to loop through all the others at all. Again, the trick is storing the current element in a variable and remove the class from that cached element:
var current = null;
document.body.addEventListener('click', function(ev) {
if (ev.target.tagName === 'BUTTON') {
if (current) {
current.classList.remove('focus');
}
current = ev.target;
current.classList.add('focus');
}
}, false); |
var current = null;
document.body.addEventListener('click', function(ev) {
if (ev.target.tagName === 'BUTTON') {
if (current) {
current.classList.remove('focus');
}
current = ev.target;
current.classList.add('focus');
}
}, false);
The code as it stands here does a different thing, agreed, but the solution is still clunky. Instead of repeating the same functionality in each handler, the better way to write this would be a setfocus() function that takes the new element to focus on as a parameter and removes the class and the focus from the current one.
To conclude
All in all, I am really disappointed to see code like this in a brand new book about bleeding edge technology using JavaScript. This is not by design, this is plain and simple laziness and bad editing. JavaScript has become incredibly powerful and is used in a lot of cases for business critical functionality. In a closed environment like a SmartTV there is nothing at all wrong about that. What is wrong though is educating new developers to write code that was maybe needed to support IE4 or Netscape2 and even back then was the lazy and hard to maintain way of doing it. If you write some instructions or tutorials, please, for the sake of the quality of the next generation of developers:
- Don’t use any inline handlers or javascript: pseudo protocol URLs. You know they are a terrible hack – if you don’t then generate your HTML in JavaScript and keep it cleaner that way.
- Limit DOM interaction to the barest minimum – it slows down your App
- Don’t loop when you can store – if you need to shift a focus around this means you need to know the last “active” element and the next one to focus. There is no need to loop over all elements in a group.
- Extensibility is key – if your script is limited to a defined number of elements and repeats them in event handlers, you cause terrible maintenance code.
