2
\$\begingroup\$

I've always struggled to write JavaScript code that I thought of as great and "correct" code. And I've struggled to find good information on how to do it.

Recently I was given a task at work to write a library for displaying an information box with info on our cookie usage. Since this was a pretty small and contained task, I thought it would be a good learning project. So I am pretty interested in getting some feedback, even on the minor things.

The library has the following public methods:

// The library holds default messages for different languages. 
// This is for overriding and adding messages
cookieAccept.setMessages()

// The library holds default button texts for different languages. 
// This is for overriding and adding button texts
cookieAccept.setButtons()

// The library defaults to using browser language. 
// This is for setting it manually
cookieAccept.setLanguage()

// The library applies some default styling, 
// so an external stylesheet is not needed. 
// This for adding a class instead. Styling will then not be applied
cookieAccept.setClass()

// Start 
cookieAccept.fire()

Implementation:

var cookieAccept = (function() {
    'use strict';

    var messages = {
            'en_US': 'We use cookies!',
            'da_DK': 'Vi bruger cookies'
        },
        buttons = {
            'en_US': 'Accept',
            'da_DK': 'Accepter'
        },
        cookieName = 'CookieAccept',
        lang = navigator.language || navigator.userLanguage,
        className = '';

    function setMessages(newMessages) {
        for (var index in newMessages) { 
            if (newMessages.hasOwnProperty(index)) {
                messages[index] = newMessages[index];
            }
        }
    }

    function setButtons(newButtons) {
        for (var index in newButtons) { 
            if (newButtons.hasOwnProperty(index)) {
                buttons[index] = newButtons[index];
            }
        }
    }

    function setLanguage(newLang) {
        lang = newLang;
    }

    function setClass(newClass) {
        className = newClass;
    }

    function setCookie(days) {
        var expires = new Date();

        expires.setDate(expires.getDate() + days);
        document.cookie = cookieName + '; ' + "expires=" + expires.toUTCString() + '; path=/';
    }

    function getCookie() {
        var cookie = document.cookie.split(';').map(function(part) {
            return part.replace(/ /g,'');
        });

        return cookie;
    }

    function setAccepted() {
        setCookie(365);

        var container = document.getElementById('acceptCookieContainer');
        container.parentNode.removeChild(container);
    }

    function hasAccepted() {
        var cookie = getCookie();

        return cookie.indexOf(cookieName) > -1 ? true : false;
    }

    function getMessage(lang) {
        return messages.hasOwnProperty(lang) ? messages[lang] : messages['en_US'];
    }

    function getButton(lang) {
        return buttons.hasOwnProperty(lang) ? buttons[lang] : buttons['en_US'];
    }

    function buildContainer(element) {
        element.style.backgroundColor = '#bbb';
        element.style.color = '#fff';
        element.style.fontFamily = 'HelveticaNeue-Light", "Helvetica Neue Light", "Helvetica Neue", Helvetica, Arial, "Lucida Grande", sans-serif';
        element.style.fontSize = '14px';
        element.style.padding = '15px';

        return element;
    }

    function buildButton(element) {
        element.style.backgroundColor = '#1793D3';
        element.style.border = 'none';
        element.style.borderRadius = '4px';
        element.style.color = '#fff';
        element.style.float = 'right';
        element.style.fontSize = '12px';
        element.style.margin = '-7px 0 0 0';
        element.style.padding = '8px 20px';

        return element;
    }

    function buildParagraph(element) {
        element.style.display = 'inline';

        return element;
    }

    function displayNotification() {
        var message   = document.createTextNode(getMessage(lang)),
            action    = document.createTextNode(getButton(lang)),
            container = className != '' ? document.createElement('div') : buildContainer(document.createElement('div')),
            button    = className != '' ? document.createElement('button') : buildButton(document.createElement('button')),
            paragraph = className != '' ? document.createElement('p') : buildParagraph(document.createElement('p'));

        button.setAttribute('id', 'acceptCookie');
        container.setAttribute('id', 'acceptCookieContainer');

        if (className != '') {
            container.setAttribute('class', className);
        }

        button.appendChild(action);
        paragraph.appendChild(message);

        container.appendChild(paragraph);
        container.appendChild(button);

        document.body.insertBefore(container, document.body.firstChild);
    }

    function fire() {
        if ( ! hasAccepted()) {
            displayNotification();  
            document.getElementById('acceptCookie').addEventListener('click', cookieAccept.setAccepted);
        }
    }

    return {
        fire: fire,
        setMessages: setMessages,
        setButtons: setButtons,
        setLanguage: setLanguage,
        setClass: setClass
    };

}());

And it would be used like this:

document.addEventListener("DOMContentLoaded", function() { 
    cookieAccept.setMessages({'en_GB': 'We eat cookies'});
    cookieAccept.setButtons({'en_GB': 'Yummi'});
    cookieAccept.setClass('cookieContainer');
    cookieAccept.setLanguage('en_GB');
    cookieAccept.fire();
}); 
\$\endgroup\$
2
\$\begingroup\$

In general looks good I think. If you really use I18N to that effect you could also add more handling to allow fallback for everything foo_* to foo_US (or whatever the "best" option is), so if some regional option isn't set, it would still fall back to at least the same language.

Some of the element names can be extracted into semi-constants, so as to be more customisable, e.g. var acceptCookieName = 'acceptCookie';.

Also, for customisation it would be cool to be able to set your own CSS. If I were a user of the library I'd really want to set different values and it would also make things easier if you were able to specify CSS classes instead.

setMessages and setButtons, as well as getMessage and getButton are basically the same; since there are only two of them each it's probably okay, but my personal threshold to deduplication is two; well, YMMV.

The hasAccepted function can be simplified to:

function hasAccepted() {
    return getCookie().indexOf(cookieName) > -1;
}

In displayNotification you have the same test for className four times; at that point I'd think about creating a new function or otherwise reduce the repetition.

In fire I'd say that adding the event listener should happen in displayNotification instead, it's part of building the widget, so it should go there.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks! That's a great answer. I'll try to work some of the suggestions in and will probably post the reviewed version as well. \$\endgroup\$ – Stromgren Nov 12 '14 at 11:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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