15
\$\begingroup\$

For my CS class, I need to, by the end, have written an entire website. The idea for my site is to supply information about Uromastyces. Because of this, most of each page will be nearly identical. The only part that changes will be the main body that contains unique information (this seems to be common on most large sites. The header and navbar are always the same, but the body can differ).

I first manually wrote up the first 3 pages, then realized there was a good number of changes I wanted to make to the layout, which required my to comb through each page of code, making identical changes everywhere.

Then I learned about DOM manipulation, and I came up with the idea of generating the page from a central template, and filling in a section (the "unique body") with the part of the page that changes.

This is the JS code that I came up with to fill this need. It's in 2 main parts, each represented as an object literal:

  • SitePieces: "Generator" functions that create the individual parts of the site (the header, the nav bar, the footer...). Any piece of changeable information is fed in.
  • SiteFactory: The section that holds all the otherwise "magic" constants that may change, or at least should be easy to change or lookup (class names, and things like the image path for the sidebar image), and puts all the pieces together.

To change the overall structure, SiteFactory is updated. To change an individual part, the relevant function in SitePieces is updated.

What I'd like advice on:

  • Right now, all my constants are just grouped together at the top of SiteFactory in a semi-arbitrary ordering. Is there a neater way to organize the data?
  • General thoughts on the setup of SitePieces and SiteFactory.
  • 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? And should I be using Hungarian-style notation (-"DOM")? I know it's generally frowned upon, but I need to communicate type information somehow.
  • ANYTHING else. I'm just learning JS now, so any advice about style/idioms/whatever would be appreciated.

What I don't want advice on:

  • From what I know about jQuery, I'm pretty sure it could have helped simplify this code a lot. I'm not quite at the point where I want to pick it up though, I'd rather learn JS more in-depth first, therefore I don't need advice to use jQuery.
  • I need to tweak the CSS. From the screenshot, you can see that some parts are squished together (the footer). That's beyond the scope of my review request though.
  • The fact that this isn't a generalized factory. I'm not (currently) looking to create a general-purpose factory to put sites together, I only need it to build my project's site.

Sample without a unique body:

Generated Webpage Sample

It's use:

<!DOCTYPE html>
<html lang="en">
<head>

</head>
<body>
    <script src="SiteGenerator.js"></script>
    <script>
        SiteFactory("Test Page", document.createElement('div'), "15.12.13");
    </script>

</body>
</html>

SiteGenerator.js:

/**
 * Created by Brendon on 2015-12-08.
 */

/*
    Constructs the site using the supplied constructor information.
 */
var SiteFactory = function(pageTitle, uniqueBodyDOM, lastModified) {
    //The various site tweaks:
    var headerClasses = 'mainHeader';

    var navLinks = [
        new LI('Home', 'index.html'),
        new LI('Enclosure', 'enclosure.html'),
        new LI('Diet', 'diet.html'),
        new LI('Behavior and Life', 'behaviorAndLife.html'),
        new LI('About Me', 'aboutMe.html')
    ];
    var navBarClasses = 'navBar';

    var sidebarImgPath = 'images/pascal-cropped-shrunk.jpg';
    var sidebarImgAlt = 'A Picture of Pascal';
    var sidebarImgClasses = 'sideBarImage';
    var sidebarClasses = 'mainSideBar';

    var uniqueBodyClasses = 'uniqueBody';

    var footerClasses = 'mainFooter';
    var footerWrapperClasses = 'footerWrappers';
    //End of tweaks

    SitePieces.appendHeadElements(pageTitle);

    var header = SitePieces.generateHeader(headerClasses);

    var navBar = SitePieces.generateNavBar(navLinks, navBarClasses);

    var sideBar = SitePieces.generateSideBar(navBar, sidebarImgPath, sidebarImgAlt, sidebarImgClasses, sidebarClasses);

    var uniqueBodyWrapper = SitePieces.generateUniqueBodyWrapper(uniqueBodyDOM, uniqueBodyClasses);

    var footer = SitePieces.generateFooter(lastModified, footerClasses, footerWrapperClasses);

    SitePieces.appendChildren(document.body,
            [header, sideBar, uniqueBodyWrapper, footer]);
};

/*
    Helper functions to generate the individual pieces of the site
 */
var SitePieces = {
    /*
        Appends the required elements to the pages's head.

        pageTitle: The title of the page to be generated.
        uniqueStyleSheetPath: A path to a unique style sheet to be used on this page (optional).
     */
    appendHeadElements: function(pageTitle, uniqueStyleSheetPath) {

        var title = document.createElement('title');
        title.innerHTML = pageTitle;

        var meta = document.createElement('meta');
        meta.charset = 'UTF-8';

        var cssLink = this.createCSSLink('styles/styles.css');

        this.appendChildren(document.head,
                [title, meta, cssLink]);

        if (uniqueStyleSheetPath) {
            var uniqueCSSLink = this.createCSSLink(uniqueStyleSheetPath);

            document.head.appendChild(uniqueCSSLink);
        }
    },

    /*
        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;
    },

    /*
        Generates the navbar for the page, populated by the supplied links

        links: The links to populate the navbar with.
        navClassName: The classes to assign to the navbar.

        returns: The generated navbar.
     */
    generateNavBar: function(links, navClassNames) {
        var nav = document.createElement('nav');
        nav.className = navClassNames;

        var unordList = document.createElement('ul');

        for (var linkI = 0; linkI < links.length; linkI++) {
            var linkObject = links[linkI];

            var link = document.createElement('a');
            link.href = linkObject.linkAddress;
            link.innerHTML = linkObject.linkText;

            var listItem = document.createElement('li');
            listItem.appendChild(link);

            unordList.appendChild(listItem);
        }

        nav.appendChild(unordList);

        return nav;
    },

    /*
        Generates the sidebar for the page

        navBar: The previously generated navbar to place inside the sidebar.
        imgSrc: The source of the image for the sidebar.
        imgAlt: A description of the sidebar image.
        sidebarImgClasses: The classes to assign to the sidebar image.
        sidebarClasses: The classes to assign to the sidebar.

        returns: The generated sidebar.
     */
    generateSideBar: function(navBar, imgSrc, imgAlt, sideBarImgClasses, sideBarClasses) {
        var aside = document.createElement('aside');
        aside.className = sideBarClasses;

        var image = document.createElement('img');
        image.src = imgSrc;
        image.alt = imgAlt;
        image.className = sideBarImgClasses;

        this.appendChildren(aside,
                [image, navBar]);

        return aside;
    },

    generateUniqueBodyWrapper: function(uniqueBodyDOM, wrapperClasses) {
        var uniqueBodyWrapper = document.createElement('section');
        uniqueBodyWrapper.className = 'wrapperClasses';
        uniqueBodyWrapper.appendChild(uniqueBodyDOM);

        return uniqueBodyWrapper;
    },

    /*
        Generates the footer for the page

        lastModifiedDate: The date of last modification of the page content
        footerDataWrapperClasses: The classes to assign to the "author" and "date modified" wrappers.

        returns: The generated footer
     */
    generateFooter: function(lastModifiedDate, footerClasses, footerDataWrapperClasses) {
        var footer = document.createElement('footer');

        var authorWrapper = document.createElement('span');
        authorWrapper.className = footerDataWrapperClasses;

        var authorLabel = document.createElement('span');
        authorLabel.innerHTML = 'Author:';

        var authorName = document.createElement('span');
        authorName.innerHTML = 'Brendon Williams';

        this.appendChildren(authorWrapper,
                [authorLabel, authorName]);

        var lastModifiedWrapper = document.createElement('span');
        lastModifiedWrapper.className = footerDataWrapperClasses;

        var lastModifiedLabel = document.createElement('span');
        lastModifiedLabel.innerHTML = 'Last Modified:';

        var lastModified = document.createElement('span');
        lastModified.innerHTML = lastModifiedDate;

        this.appendChildren(lastModifiedWrapper,
                [lastModifiedLabel, lastModified]);

        this.appendChildren(footer,
                [authorWrapper, lastModifiedWrapper]);

        return footer;
    },

    /*
        Helper function to append the list of children to the parent.
     */
    appendChildren: function(parent, elements) {
        for (var i = 0; i < elements.length; i++) {
            parent.appendChild(elements[i]);
        }
    },

    createCSSLink: function(cssPath) {
        var cssLink = document.createElement('link');

        cssLink.rel = 'stylesheet';
        cssLink.type = 'text/css';
        cssLink.href = cssPath;

        return cssLink;
    }

};

/*
    A list-item object to assist in passing links.
 */
var LI = function(linkText, linkAddress) {
    this.linkText = linkText;
    this.linkAddress = linkAddress;
};
Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
10
\$\begingroup\$

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.stack.imgur.com/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'))
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ I like your comment on jQuery, it's a good observation \$\endgroup\$
    – Alex L
    Dec 14, 2015 at 19:10
7
\$\begingroup\$

This is normal behavior, especially to newcomers to the web - the "do everything in JavaScript after you discover its power".

ANYTHING else. I'm just learning JS now, so any advice about style/idioms/whatever would be appreciated.

Sometimes it's just best done where it should be done - markup. So skipping everything, I would suggest you take a look at Mustache. It's a simple templating library (it only interacts with your code in one line). Given a data object (which could contain everything dynamic inside your site), you pop in a template and poof! Page! It also makes you aware of the separation between presentation and the data.

http://codepen.io/anon/pen/yeYZEP

<!-- define the container of the page. -->
<div id="container"></div>

<!-- If you put an unknown type, the browser won't execute it as a script -->
<!-- Makes it a good candidate for putting stuff you want to retrieve later -->
<script type="not/javascript" id="template">
  <div class="header">{{ title }}</div>
  <div class="contents">{{ page }}</div>  
</script>

<script>

  // TL;DR: 
  // - Provide data
  // - Get template
  // - Get container
  // - Render the page
  // - Put page in container

  var data = {
    title: 'Hello World',
    page: 'Some random text'
  };

  var template = document.getElementById('template').innerHTML;
  var container = document.getElementById('container');

  // The only part you see a foreign API in your code - to generate
  // markup from a template and your data.
  var page = Mustache.render(template, data);

  container.innerHTML = page;

</script>

With the approach above, you can easily visualize what your markup is going to look like while keeping data separate from it. Mustache also has constructs for repeating items, handy for building menus from an array. It also has "partials" - reusable markup which comes handy when you use a common header, footer, or sidebar.

Now for the rest of your code. Mustaches work great with body content. I've never seen people use it to prep the <head> (except in server-side rendering cases). Just a little warning. Your mileage may vary if you try doing it.

createCSSLink: function(cssPath) {
    var cssLink = document.createElement('link');

    cssLink.rel = 'stylesheet';
    cssLink.type = 'text/css';
    cssLink.href = cssPath;

    return cssLink;
}

Loading CSS dynamically this way is unreliable as <link> elements have wonky error and load events. So you can't really rely this method. An alternative is using AJAX to load the sheet, but that's taking it too far.

What I suggest for this small project is to just put everything in regular stylesheets and put them in a page. Single or multiple files, that's up to you. But what I do suggest is to read up on BEM. It's a way to organize your CSS styles in a way that huge stylesheets won't start killing each other with !important and load order.

Right now, all my constants are just grouped together at the top of SiteFactory in a semi-arbitrary ordering. Is there a neater way to organize the data?

Yes. Put them in separate files. For a small project, I'd worry less about HTTP latency.

Are my comments good?

Comments are always good, no matter what form. There's also JSDoc which looks like JavaDoc. Also, comments should not tell the story of what is being done, but instead tell why it's being done. Code like the following would make no sense:

// Setting references to null
foo.bar = null;

That doesn't really tell me anything other than the fact that its repeating what I already know. Besides, why null? Why foo.bar? Why do it now?

// Setting references to null for garbage collection thingy
foo.bar = null;

With this contrived example, we know that foo.bar was set to null for GC purposes, and not anything else.

A green background? Really? Also, Times New Roman is so year 2000. Use Open Sans. :D

Improve this answer
\$\endgroup\$
3
\$\begingroup\$

If you want to make your site builder really dynamic, you shouldn't keep the page variables inside the constructor, use an extend like function and keep them as external options.

SitePieces should not be an object, you should use the functions in a prototype mould, for example:

var SitePieces = function(){};

SitePieces.prototype.appendHeadElements = function(){
    // ...
};

and so on.

LI:

LI should actually make the links, not just assign properties...

var navLinks = [
    new LI('Home', 'index.html'),
    new LI('Enclosure', 'enclosure.html'),
    new LI('Diet', 'diet.html'),
    new LI('Behavior and Life', 'behaviorAndLife.html'),
    new LI('About Me', 'aboutMe.html')
];
var LI = function(linkText, linkAddress) {
    this.linkText = linkText;
    this.linkAddress = linkAddress;
};

into:

var navLinks = [
    LI('Home', 'index.html'),
    LI('Enclosure', 'enclosure.html'),
    LI('Diet', 'diet.html'),
    LI('Behavior and Life', 'behaviorAndLife.html'),
    LI('About Me', 'aboutMe.html')
];
var LI = function(linkText, linkAddress) {
    var link = document.createElement('a');
    link.href = linkAddress;
    link.innerHTML = linkText;
    var listItem = document.createElement('li');
    listItem.appendChild(link);
    return listItem;
};

Then generateNavBar becomes:

var nav = document.createElement('nav');
var unordList = document.createElement('ul');

nav.className = navClassNames;
unordList.appendChild(links);
nav.appendChild(unordList);
return nav;
Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.