Skip to main content
removed secret santa reference as it is irrelevant to the post
Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155

I would write more, but I am being pestered to go sign up for the secret santa. :(

I would write more, but I am being pestered to go sign up for the secret santa. :(

edited body
Source Link
Quill
  • 12.1k
  • 5
  • 41
  • 94

Are my comments good? I'm coming from Java, and tried to use a Javadoc-like notation, but there doesn't seem to be any way to access the comments besides manually checking the source.

Are my comments good? I'm coming from Java, and tried to use a Javadoc-like notation, but there doesn't seem to be any way to access the comments besides manually checking the source.

Should I be doing type checks? Say, for the function generateUniqueBody, should I be checking that uniqueBodyDOM is in fact a DOM object?

Should I be doing type checks? Say, for the function generateUniqueBody, should I be checking that uniqueBodyDOM is in fact a DOM object?

Are my comments good? I'm coming from Java, and tried to use a Javadoc-like notation, but there doesn't seem to be any way to access the comments besides manually checking the source.

Should I be doing type checks? Say, for the function generateUniqueBody, should I be checking that uniqueBodyDOM is in fact a DOM object?

Are my comments good? I'm coming from Java, and tried to use a Javadoc-like notation, but there doesn't seem to be any way to access the comments besides manually checking the source.

Should I be doing type checks? Say, for the function generateUniqueBody, should I be checking that uniqueBodyDOM is in fact a DOM object?

Source Link
Dan
  • 3.8k
  • 24
  • 39

Let's start on a few of the things you explicitly asked questions on before moving over to the code itself.

Are my comments good? I'm coming from Java, and tried to use a Javadoc-like notation, but there doesn't seem to be any way to access the comments besides manually checking the source.

There are a few ways of using comments but most of them stem from some version of JSDoc. JSDoc compliant comments can have a command line tool run over them that parses the comments and generates a web page for those comments. That might be a bit overkill for this scenario, but it does exist and is used widely - most notably by AngularJS (note that that code is in TypeScript, which is a dialect of JavaScript, so don't expect to understand all of it at face value. The comments are what are important).

This documentation becomes this website. Again, this is slightly overkill for a simple project like you have, but it's worth knowing nonetheless.

Should I be doing type checks? Say, for the function generateUniqueBody, should I be checking that uniqueBodyDOM is in fact a DOM object?

You probably shouldn't be checking types per se, and even if you were, I would recommend an alternative dialect of JavaScript which will cover that purpose much more thoroughly (this is what TypeScript/FlowJS et al try to solve). You should be checking whether or not the thing you are accessing has the properties you need. This is how we implement backwards compatible functionality in JavaScript too (some browsers may not support the functions we're using, so they will be undefined).

That said, in the scenario you have (generateUniqueBodyWrapper), it is not really feasible to be checking whether or not uniqueBodyDOM is an element or not, so I would recommend simply just making sure that it is not falsey (which is null or undefined). That is something as simple as this:

function(uniqueBodyDOM, wrapperClasses) {
  if (!uniqueBodyDOM) {
    // If uniqueBodyDOM does not exist, nothing will happen.
    // You could alternatively throw an error here if you so wished.
    return;
  }
  var uniqueBodyWrapper = document.createElement('section');
  uniqueBodyWrapper.className = 'wrapperClasses';
  uniqueBodyWrapper.appendChild(uniqueBodyDOM);

  return uniqueBodyWrapper;
}

JavaScript will throw its own error if you try and append a non-DOM element to the DOM, so there's not much point in you doing that as well.

Let's talk jQuery

I'm glad you said you don't want to learn jQuery just now. I'm a bit bias (currently dealing with a lot of hoo-hah at work due to jQuery and its silliness in work at the moment) but I just want to say to you (and to anyone else reading this) that you might not need jQuery and jQuery is very dangerous when inter-operating between different DOM frameworks (for example - Angular and jQuery) due to the way it internally works by caching elements.

Browsers can make great optimisations on stuff like document.getElementByXXX such as caching the element so it is O(1) after first access. There's almost never a reason to use jQuery unless you're using a framework that requires it - such as Backbone - or laziness, these days. Or unless you're supporting dinosaur-age browsers.

Don't get me wrong, developer productivity is definitely better than speed, but jQuery is in a bit of a strange place at the moment where it doesn't make sense to use jQuery on small sites (because the modern API is pretty concise already) and it doesn't make sense to use it on large sites (because frameworks like Angular and React are really good at that). It's basically in a place where it works either as a compatibility layer or as a productivity aid for mid-level sites. Unless you're sure you're going to be making a mid-level site, you should probably just stick to vanilla or framework.

And you should definitely know vanilla before learning jQuery or a framework. As the site says, though, you should know what jQuery is doing for you - and what it isn't.

And should I be using Hungarian-style notation (-"DOM")? I know it's generally frowned upon, but I need to communicate type information somehow.

No. This is a widely reached consensus in the programming community in nearly every language. [citation needed] /thread

Name your functions.

You've got a lot of anonymous functions in your code that are assigned to an object. While in future versions of JavaScript the engine may be able to infer the name of the function based on the key it is assigned to (Babel already does this!), it is a good idea to assign names to your functions. The main benefit of this is that they will show up in the debugger with those names instead of (anonymous), making debugging significantly easier.

Example:

https://i.sstatic.net/0lOUO.png


On to the code.

I'm going to second what was said in @JosephTheDreamer's answer and suggest that you use a templating library - it'll make it harder for you to lose your sanity later on in the development when you keep having to change things. Creating DOM elements from scratch in JavaScript can be a miserable experience, but it is also quite slow. Frameworks such as Angular take an approach where you create the DOM once and then you iterate over it with new data sets when the data changes. This is what a templating library will help you do.

@JosephTheDreamer suggested Mustache, which is a great library, however I prefer John Resig's templates. These are lightweight but also come with both Lodash and Underscore which are commonly used JavaScript utility belts. You can change the template delimiters, but they mostly look like this:

<template id='template'>
  <header class='header'><%= title %></header>
  <section class='contents'><%= page %></section>
</template>

Note that I've used <template> instead of <script>. This is the semantically correct HTML5 elements, though some browsers may not support it in which case you should use a polyfill or use the <script> tags that @JosephTheDreamer used originally. Essentially all a template tag does is store inert HTML that does nothing on its own.

This can be used like the following:

var data = {
  title: 'Hello, world',
  page: 'Some random text'
}
var template = document.getElementById('template')
var linkFn = _.template(template.innerHTML)

// This is the node which is actually rendered.
var node = template.cloneNode()
node.innerHTML = linkFn(data)
document.body.appendChild(node)
// You could of course do what Joseph did and just set the innerHTML of an already existing Node instead of cloning & so forth.

On creating <link> elements with JavaScript

Don't do that - Joseph has already stated the reasons for this (Sorry to lean on your answer so much). There are a couple of ways of changing CSS in JavaScript - either have a predefined list of classes you've already written in CSS that you then apply to the document or we modify inline styles (be careful, this breaks the C (cascade) in Cascading Style Sheets - although proponents of the BEM methodology don't see this as an issue).

Separate files vs single file

With the introduction of tools such as webpack, there's no real reason to put things in a single file (outside of really rapid prototyping). Split things up in a manner which makes sense to other developers (and, most importantly, to yourself). You can combine them using a build process through a multitude of command line tools - either with a simple concatenation script that can be done on the command line all the way up to a complex build process involving gulp and webpack.

Multiple files will increase how modular your code is, making it easier to read and easier to maintain.

SRP

This is partially solved by the template engine that both myself and Joseph have mentioned, but a lot of your functions right now are doing, well, a lot of things and are not doing one single thing. This makes your code harder to reason about, test and by extension maintain. It's a good idea to try and learn early on to keep as much separation between DOM and logic as you can. Logic can be tested independently of DOM access and may change in numerous ways across the lifetime of your application; mixing DOM with Logic will just end up with a headache. You want to reduce the place where your application touches the DOM in as few places as possible.

Of course, libraries like React challenge this assertion, but they don't do DOM access in their components - they simply invoke functions which describe DOM access (ah, so that's how Monads work in Haskell...), making them still be able to be tested.

For example, let us take a look at one of your functions:

/*
    Generates the page header

    headerClass: The classes to assign to the header
 */
generateHeader: function(headerClasses) {
    var header = document.createElement('header');
    var h1 = document.createElement('h1');

    h1.innerHTML = "Brendon's Uromastyces Fact Site";

    header.appendChild(h1);

    return header;
}

This is doing a number of things;

  • It creates the DOM elements for the header and title
  • It knows about the title for the header
  • It knows the layout of the DOM structure for the header itself

What if you want to change the header title or DOM structure? You don't really want to have to modify this function to do both of those things, and you also now cannot test how the title is rendered in isolation - you have to test the entire header. With the templating library, things look a bit different:

<template id='header'>

</template>

// Note that I've removed the class aspect because generateHeader wasn't using headerClasses anyway!
/**
 * Generates the DOM layout for the page header.
 * @param {String} title The title of the page.
 */
function generateHeader(title) {
  // This could be separated even *further* if you wanted.
  // Note that getElementById is already cached by the browser.
  var template = document.getElementById('header').innerHTML
  return _.template(template)({ title: title })
}

document.appendChild(generateHeader('Brendon\'s Uromastyces Fact Site'))

I would write more, but I am being pestered to go sign up for the secret santa. :(